krausest / js-framework-benchmark

A comparison of the performance of a few popular javascript frameworks
https://krausest.github.io/js-framework-benchmark/
Apache License 2.0
6.56k stars 811 forks source link

Add mogwai #1157

Closed flosse closed 1 year ago

flosse commented 1 year ago

@schell I never build a mogwai app so would you mind to have a look at this draft?

ToDos:

schell commented 1 year ago

Oh wow, cool! I'll take a look.

schell commented 1 year ago

@flosse this looks great! Does it work?

Unfortunately (or fortunately) "idiomatic mogwai code" doesn't exist yet! This looks easy enough to follow, which is really the only requirement in my book.

So awesome to see you make this so fast! How was your experience writing this in mogwai? Was it easy enough to figure out how to do what you needed to do?

schell commented 1 year ago

I made a PR into your branch with some improvements. I'm not used to npm though, so I might have made some mistaken changes with regard to the packaging.

flosse commented 1 year ago

I'm not used to npm though

I used npm (and JS) several years and I still hate it, which is why I'm interested in rust alternatives :wink:

so I might have made some mistaken changes with regard to the packaging.

looks good so far and even better because I myself had not even run the npm script yet :rofl:

flosse commented 1 year ago

How was your experience writing this in mogwai? Was it easy enough to figure out how to do what you needed to do?

well, i'm used to working with TEA and seed in particular which is why I had to change briefly but overall it was a nice experience. The only extraordinary thing for me is the ListPatchModel.

flosse commented 1 year ago

@krausest I have trouble running npm run isKeyed keyed/mogwai:

npm run isKeyed keyed/mogwai

> webdriver-ts@1.0.0 isKeyed
> cross-env LANG="en_US.UTF-8" ts-node  --files  src/isKeyed.ts keyed/mogwai

args.framework undefined true
Frameworks that will be checked
*** headless undefined

BTW: Why are the benchmarks distinguished in such a way? It would be cool to compare e.g. a non-keyed framework like seed directly with a keyed one like sycamore.

schell commented 1 year ago

I've had a hard time figuring out what "keyed" and "non-keyed" means. @krausest and @flosse what does it mean, exactly?

krausest commented 1 year ago

I wrote an article about keyed vs. non-keyed here: https://www.stefankrause.net/wp/?p=342

krausest commented 1 year ago

The isKeyed check should actually just work. In your case it seems like keyed/mogwai couldn't be found in the list of all available frameworks. The list of the frameworks can be seen on http://localhost:8080/ls . Does its output contain an entry for mogwai? I ran the isKeyed check on the latest commit of this PR and I got the following result:

npm run isKeyed keyed/mogwai

> webdriver-ts@1.0.0 isKeyed
> cross-env LANG="en_US.UTF-8" ts-node  --files  src/isKeyed.ts keyed/mogwai

args.framework undefined true
Frameworks that will be checked mogwai-v0.6.0-keyed
*** headless undefined
Keyed test for swap failed. Swap must add the TRs that it removed, but there were 2 new nodes
mogwai-v0.6.0-keyed is keyed for 'run benchmark' and keyed for 'remove row benchmark' and non-keyed for 'swap rows benchmark' . It'll appear as non-keyed in the results
ERROR: Framework mogwai-v0.6.0-keyed is not correctly categorized

So currently swap isn't keyed.

Some remarks:

schell commented 1 year ago

@krausest how did you run the benchmark to get 15775 msecs - so I can reproduce and troubleshoot? Typically mogwai is quite fast, so I'm surprised.

krausest commented 1 year ago

BTW: Why are the benchmarks distinguished in such a way? It would be cool to compare e.g. a non-keyed framework like seed directly with a keyed one like sycamore.

It wouldn't give any sensible results. non-keyed swap might just exchange the text of two rows, keyed must actutally move the dom nodes. non-keyed remove rows might move the text of all rows below the deleted row one row up, keyed must remove the row (which causes more css recomputations). So you'd compare very different dom actions and thus you couldn't tell much about the performance between the frameworks. You could compare non-keyed and keyed for the same framework and see which strategy is faster. For this benchmark the alternating row color helps the non-keyed implementations which will not always be the case. Still I'd always prefer an implementation that supports keyed updates (since all updates are then correct) and maybe consider switching to non-keyed updates (i.e. use the row index as the key) when it helps the performance and doesn't cause any errors.

krausest commented 1 year ago

I ran npm run bench keyed/mogwai from commit 48c5ae298417dd4498cdb010e5aaa52bffd42e81. This is a preview of what I got:

Bildschirm­foto 2023-01-09 um 22 32 43
schell commented 1 year ago

After reading the keyed vs non-keyed article I think I can say that this implementation is non-keyed, but also doesn't do what other non-keyed implementations do. Mogwai is capable of both, so I'll give a try when I have a moment.

schell commented 1 year ago

I got the benchmark running and it's pretty much the same over here! There's definitely something very odd about the "create many rows" benchmark. Thanks for the insight, I'll be working on this in my free time.

flosse commented 1 year ago

@krausest thanks for the explanations and the link to your blog post!

The isKeyed check should actually just work. In your case it seems like keyed/mogwai couldn't be found in the list of all available frameworks.

exactly!

The list of the frameworks can be seen on http://localhost:8080/ls . Does its output contain an entry for mogwai?

no :disappointed: But never mind, then it must be somehow related to my setup :see_no_evil:

I hope you understand that I can't merge the PR until if mogwai becomes fast enough to complete without the timeout.

:+1:

@schell I'd say we close this PR and create a new one a soon as the issues are solved.

schell commented 1 year ago

@krausest just so I can confirm that I've fixed the issues - what method does this benchmark use to verify that 10k rows have been created?

krausest commented 1 year ago

It checks that the following selector can be found: tbody>tr:nth-of-type(10000)>td:nth-of-type(2)>a

schell commented 1 year ago

@flosse I'm very glad you added mogwai to this benchmark because I found a bunch of debugging strings and extra closures that I meant to clean up, that somehow made it into the 0.6 release (probably because it was such a long time between 0.5 and 0.6. Needless to say I think I'll have a pretty good performing implementation of this benchmark this weekend. 🤞

flosse commented 1 year ago

Hey @schell, that makes me happy to read :smile: And I'm curious to see how mogwai will develop further. Unfortunately my free time is also very limited, otherwise I'd love to contribute more.

schell commented 1 year ago

@krausest sorry to keep pestering you on this, but what is the series of invocations I should do to clean (one framework), rebuild (one framework), bench (one framework) and display results (of all frameworks with results)? I'm having a hard time getting it right, I think.

krausest commented 1 year ago

I think that's worth writing up in the discussions: https://github.com/krausest/js-framework-benchmark/discussions/1162 Does that help?

P.S: I didn't have the time to run the commands right now so I hope there aren't too many typos in there.

schell commented 1 year ago

Ah yes! Thanks, that clears things up quite a bit. @krausest @flosse I'll let y'all know when I have something workable. Thanks again :)