-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benchmark crate for core features #3487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, just a few comments
- name: Run master benchmark | ||
run: cargo bench -q > ../output.log | ||
continue-on-error: true | ||
working-directory: yew-master/tools/benchmark-core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably cache this somewhere (perhaps in firestore db or storage bucket, since we already use firebase) but this is fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt it already cached by the 2 previous steps (swatinem/rust-cache)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I was thinking of just downloading the expected output file so benchmark is never run when it can be avoided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... well. tbh I dont think it's worth the effort haha
@@ -0,0 +1,14 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why not put it in yew's benches instead of creating a new crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know... 🤷♀️ well I guess I can simply move them there I guess. I tried to keep the directory architecture with the other benchmarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what do you prefer? should I move the bench in the crate yew
? or keep with a separate crate for consistency sake? both are fine imo i don't really care
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. It doesn't really make that much of a difference and moving it now is just extra work
<div class={classes!("hello-world")}> | ||
<span>{"Hello"}</span> | ||
<strong style="color:red">{"World"}</strong> | ||
<Stuff /> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We clone only VTag and VComp here. Probably wanna benchmark the others too, especially VList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure VList is in there because there is a list of nodes.
I'm also cloning classes and style (which are also very important imo).
(and yes the <Stuff/>
thingy is purely to get a VComp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ah and I'm also cloning VText (Hello and World are VText I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. It's still missing a few cases though: VSuspense and VRef. Not sure if we can benchmark those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cecton can we benchmark those or should we just merge it in as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add VSuspense but not VRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Feel free to merge it once (relevent) CI is green
Description
Add a tool crate benchmark-core where we can run benchmarks on some basic core
features like VNode.
Related to #3431
Checklist