Skip to content
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

add maquette #270

Closed
leeoniya opened this issue Nov 1, 2017 · 8 comments
Closed

add maquette #270

leeoniya opened this issue Nov 1, 2017 · 8 comments

Comments

@leeoniya
Copy link
Contributor

leeoniya commented Nov 1, 2017

hey @johan-gorter, now that maquette v3.0 is out, is there any interest in joining the party? :)

this bench is pretty easy to implement but if you dont have time, i can try to slap something together and you can tune it as needed.

AFASSoftware/maquette#38

@johan-gorter
Copy link
Contributor

Sure we want to join this party. I currently do not have time to get much work done, but it should be enough to do the tuning. I already implemented the uibench benchmark, so you can find implementation clues here.

From my experience with uibench, I know that maquette is neither the fastest nor the slowest framework when it comes to pure DOM manipulation. It should be one of the fastest when there are no changes. We also want to build a feature which allows you to minimize costly reflows when measuring DOM nodes.

@leeoniya
Copy link
Contributor Author

leeoniya commented Nov 2, 2017

i made a keyed impl here: #271.

overall perf looks quite good. "select row" is a bit of an outlier. i also noticed that swap rows appears to touch every node in between.

@johan-gorter
Copy link
Contributor

I can confirm that maquette does not reuse DOM nodes when swapping, but it can still animate a swap. A long in-depth discussion about this can be found here: AFASSoftware/maquette#13.

@johan-gorter
Copy link
Contributor

A non-keyed implementation is not allowed in maquette, this is maquette's rule#2

@johan-gorter
Copy link
Contributor

Question: maquette's projector uses requestAnimationFrame to delay rendering to make sure it never renders more than 60 frames per second. I see that the benchmark uses the projector, so how can you measure how long the rendering took exactly?

@leeoniya
Copy link
Contributor Author

leeoniya commented Nov 2, 2017

It should be one of the fastest when there are no changes.

i actually proposed adding this metric previously in #163 because i thought it would be valuable as well, but select row gets close enough to "do nothing"

I see that the benchmark uses the projector, so how can you measure how long the rendering took exactly?

this bench uses selenium and chromedriver to extract metrics from the timeline, so it doesnt matter whether ops are synchronous or async since the measurement is not done in userland.

A non-keyed implementation is not allowed in maquette, this is maquette's rule#2

since maquette focuses heavily on supporting transitions, i can see why non-recycling needs to be enforced. that's perfectly fine.

@johan-gorter
Copy link
Contributor

Hello,

I finally had a chance to run the benchmark locally. It looks like using requestAnimationFrame or not is not affecting the outcome significantly. This means that the benchmark does a good job at approximating real world usage!

About the 'outlier' select-row. Maquette does something inefficient when handling events, because it navigates through the real dom and virtual dom to find the right event handler to invoke. In our experience this inefficiency always takes under a millisecond, so it should not be noticeable. I expect this gets enlarged somehow by this benchmark, although I do not yet see how. The benchmark outcome suggests maquette takes about 36ms to respond to the click but when I profile locally, it is usually about 13ms. (assuming 1000 rows)

I hope this pull request gets merged, so we have a baseline that we can check regularly.

By the way, I somehow had to disable uglify in build.js, because it created errors at runtime (getProjection was undefined).

@krausest
Copy link
Owner

Fixed in 6d33ecf
Glad to see that a keyed framework is added!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants