js-framework-benchmark icon indicating copy to clipboard operation
js-framework-benchmark copied to clipboard

Add Million.js benchmark

Open jasonappah opened this issue 1 year ago • 11 comments

This PR adds benchmarks for Million.js' Virtual DOM and its React compatibility layer. See https://millionjs.org for more info. cc @aidenybai

jasonappah avatar Aug 02 '22 12:08 jasonappah

Thanks. Read about it on hacker news and reddit and was looking forward getting a PR ;-)

But it seems like we have to work a few things out.

There appears to be an issue with non-keyed/million-react: Click "create 1,000 rows" and then "clear". The following error is reported on the console

index.c37c6877.js:formatted:461 Uncaught RangeError: Invalid array length
    at Proxy.<anonymous> (index.c37c6877.js:formatted:461:26)
    at HTMLButtonElement.r (index.c37c6877.js:formatted:963:11)

The following issue was found for keyed/million and non-keyed/million: Selecting a row by clicking on it doesn't highlight it.

keyed/million-react fails for the isKeyed test:

npm run isKeyed -- --headless true keyed/million-react
...
Expected to find 'aria-hidden'=true on span in third td, but found  null
ERROR: Framework million-react-v1.12.3-beta.1-keyed html is not correct
Keyed test for swap failed. Expected at least 1 added and 1 removed TR, but there were 0 added and 0 removed
Keyed test for create rows failed. Expected that 1000 TRs should be removed and added, but there were 0 added TRs and 0 were removed
Keyed test for remove failed. Expected that the dom node for the 2nd row would be removed, but it wasn't
million-react-v1.12.3-beta.1-keyed is non-keyed for 'run benchmark' and non-keyed for 'remove row benchmark' and non-keyed for 'swap rows benchmark' . It'll appear as non-keyed in the results

Looks like the implementation isn't keyed. Create 1,000 rows must create fresh rows, swap must move the dom nodes, remove must remove the row one clicked on. All those are requirements for a keyed implementation.

Can you take a look at that please?

krausest avatar Aug 02 '22 19:08 krausest

Checking it out now.

jasonappah avatar Aug 02 '22 23:08 jasonappah

The benches run successfully but don't pass the isKeyed test right now. I have an idea as to why so I'll push up a fix in the next day.

jasonappah avatar Aug 04 '22 05:08 jasonappah

I'm not sure what the state of this PR is. Should I add a draft label or should I try to merge it?

krausest avatar Aug 13 '22 20:08 krausest

@jasonappah pinging for PR status update

aidenybai avatar Aug 13 '22 20:08 aidenybai

Marking as draft, need to figure out a couple issues :)

jasonappah avatar Aug 17 '22 03:08 jasonappah

@jasonappah, Do you already have results yet to show?

frederikhors avatar Aug 21 '22 20:08 frederikhors

I snapped this code out of curiosity and ran some benches on my machine. If you don't mind me inquiring, what's left to figure out? The bench results I got on my machine, and the results were generally a regression in performance compared to React with hooks.

image

akutruff avatar Aug 31 '22 15:08 akutruff

@akutruff is both keyed and non-keyed functioning right now and https://github.com/krausest/js-framework-benchmark/pull/1074#issuecomment-1203140750 has been resolved? I'm pretty sure we've fixed this on our end in the Million repo with latest versions.

(ccc @jasonappah)

aidenybai avatar Aug 31 '22 21:08 aidenybai

I did not check unkeyed. I suggest assuming I did nothing and go through all the check in the docs.

akutruff avatar Aug 31 '22 22:08 akutruff

I'm going to go ahead and remove the keyed benchmarks for now to be revisited at some point. They run successfully, but appear not to pass the keyedness test. PR should be good to merge, apologies for the delay.

jasonappah avatar Sep 14 '22 19:09 jasonappah

Thanks. Usually I don't post any screenshots for non-keyed results, but I made an exception. I'm afraid to say it looks slower than react and vue: Bildschirmfoto 2022-09-27 um 10 01 09 PM

krausest avatar Sep 27 '22 20:09 krausest

Thanks for your patience and feedback for getting this merged! Now it's time to make it faster

aidenybai avatar Sep 27 '22 20:09 aidenybai