krausest/js-framework-benchmark

Strengthen isKeyed test for swap rows

krausest opened this issue · 18 comments

krausest commented

As discovered in #693 currently the isKeyed test for swap rows is too weak to detect implementations that remove the two rows and simply insert two new rows.
I really think it makes much more sense to require a keyed implementation to actually move the rows, i.e. add exactly the rows that were removed (otherwise any dom state would be lost when swapping rows).

If isKeyed checks whether the rows are actually moved for swap rows the following implementations fail:

  • aurelia-v1.3.0-keyed
  • binding.scala-v10.0.1-keyed
  • crui-v0.1.0-alpha.13-keyed
  • datum-v0.12.2-keyed
  • dojo-v6.0.4-keyed
  • fidan-v0.0.23-keyed
  • maquette-v3.3.0-keyed
  • mikado-v0.7.57-keyed
  • ractive-v1.3.6-keyed
  • react-hookstate-v16.8.6 + 1.5.2-keyed
  • reflex-dom-v0.4-keyed
  • san-v3.7.7-keyed

Can you please take a look at your implementations? Is it possible to fix it for your frameworks?
@Alexander-Taran @Atry @iazel @MartinRixham @agubler @ismail-codar @johan-gorter @ts-thomas @alexfmpe @errorrik

Atry commented
leeoniya commented

the name of the metric has always been "swap", unambiguously. this simply improves the assertion of the metric.

krausest commented

@Atry I don't think we're changing the definition of keyed, but indeed we're changing the test for keyed that wasn't sufficient (and I'm not sure it's now watertight).

johan-gorter commented

Maquette does not support swapping nodes, you can exclude maquette from this test for this reason.

krausest commented

I decided to update the results table such that it show any open issues for the implementations. The information is kept in package.json (js-framework-benchmark.issues).

ts-thomas commented

Hello @krausest, the mikado keyed implementation was tagged with the issue "Keyed implementations must move the DOM nodes for swap rows", but that's not right. The implementation must assign the new values to the data array (like all others).

.route("swaprows", () => {
    const tmp = store[998];
    store[998] = store[1];
    store[1] = tmp;
})

Edit: Sorry but I misunderstood the issue, I thought that the description explains the issue not the desired behavior.

krausest commented

@ts-thomas Please try the following: Click on "Create 1,000 rows", open chrome's dev-tools and set the background color of the row with id 2 to red and click on "Swap rows". If you look now at the second to last row (which has now id 2) is it still red? If not it's not keyed.

  • 👍1
krausest commented

@agubler I just noticed that the swap operation is non-keyed again for dojo 8 alpha 1.Can you take a look at it?

agubler commented

@krausest have you updated the test?

krausest commented

@agubler No. I wonder too how that happened. When updating a library I'm running the isKeyed test automatically. So either I ignored the error or it wasn't there at that time.
When checking chome 88 results I ran isKeyed for all frameworks and I got

Frameworks that will be checked dojo-v8.0.0-alpha.1-keyed
Keyed test for swap failed. Swap must add the TRs that it removed, but there were 2 new nodes
dojo-v8.0.0-alpha.1-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 dojo-v8.0.0-alpha.1-keyed is not correctly categorized
agubler commented

@krausest very strange. I’ll try and take a look over the weekend

krausest commented

@agubler Thanks. (I checked the issue manually in the dev tools. If you assign a background-color style in the dev tool to row #2 and then swap the background color it is lost. So the isKeyed check is correct.)

agubler commented

@krausest thank you for double checking

agubler commented

@krausest Hmmm this is strange I have just pulled the latest, re-installed deps, done re-builds and ran npm run check keyed/dojo and it passed! Did I do something wrong? This is on chrome 87 though, I'll try chrome 88

http://localhost:8080/frameworks/keyed/dojo/package-lock.json
Frameworks that will be checked dojo-v8.0.0-alpha.1-keyed
dojo-v8.0.0-alpha.1-keyed is keyed for 'run benchmark' and keyed for 'remove row benchmark' and keyed for 'swap rows benchmark' . It'll appear as keyed in the results

Edit: Passes in 88 too

krausest commented

Something is indeed very strange. The version in my docker container behaves non-keyed, when I build it directly on my linux machine it behaves keyed. The package-lock.json is the same for both (but I tried to clean up package-lock.json handling in this release so I'm double checking if I did something wrong there). I'll investigate and report back.

krausest commented

The build script delete many output directories, but not output. Dojo uses a directory output. I noticed that on my docker container the output directory contains multiple bootstrap..js and main..js files:

total 616
-rw-r--r-- 1 root root     70 Jan 22 21:22 bootstrap.0b93815727c9831545bc.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 21:22 bootstrap.0b93815727c9831545bc.bundle.css.map
-rw-r--r-- 1 root root   8741 Jan 22 21:22 bootstrap.b81f915a0b0556427479.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 21:22 bootstrap.b81f915a0b0556427479.bundle.js.map
-rw-r--r-- 1 root root   8741 Jan 22 21:22 bootstrap.bfe59d369934154bc858.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 21:22 bootstrap.bfe59d369934154bc858.bundle.js.map
-rw-r--r-- 1 root root     70 Jan 22 21:22 bootstrap.f484fefd59c714a3fe28.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 21:22 bootstrap.f484fefd59c714a3fe28.bundle.css.map
-rw-r--r-- 1 root root    532 Jan 22 21:22 index.html
-rw-r--r-- 1 root root  31406 Jan 22 21:22 main.9014b7391656d8c72de7.bundle.js
-rw-r--r-- 1 root root 200795 Jan 22 21:22 main.9014b7391656d8c72de7.bundle.js.map
-rw-r--r-- 1 root root  32477 Jan 22 21:22 main.b0bad89716edfc7e672a.bundle.js
-rw-r--r-- 1 root root 207891 Jan 22 21:22 main.b0bad89716edfc7e672a.bundle.js.map
-rw-r--r-- 1 root root   1792 Jan 22 21:22 manifest.json
drwxr-xr-x 2 root root   4096 Jan 22 21:22 runtime

The index.html references bootstrap.b81f915a0b0556427479.bundle.js

When I delete the output folder and let it rebuild in the docker container has the following files:

total 324
drwxr-xr-x 3 root root   4096 Jan 22 20:27 ./
drwxr-xr-x 4 root root   4096 Jan 22 20:27 ../
-rw-r--r-- 1 root root   8741 Jan 22 20:27 bootstrap.bfe59d369934154bc858.bundle.js
-rw-r--r-- 1 root root  45544 Jan 22 20:27 bootstrap.bfe59d369934154bc858.bundle.js.map
-rw-r--r-- 1 root root     70 Jan 22 20:27 bootstrap.f484fefd59c714a3fe28.bundle.css
-rw-r--r-- 1 root root    102 Jan 22 20:27 bootstrap.f484fefd59c714a3fe28.bundle.css.map
-rw-r--r-- 1 root root    532 Jan 22 20:27 index.html
-rw-r--r-- 1 root root  32477 Jan 22 20:27 main.b0bad89716edfc7e672a.bundle.js
-rw-r--r-- 1 root root 207891 Jan 22 20:27 main.b0bad89716edfc7e672a.bundle.js.map
-rw-r--r-- 1 root root   1792 Jan 22 20:27 manifest.json
drwxr-xr-x 2 root root   4096 Jan 22 20:27 runtime/

the index.html references bootstrap.bfe59d369934154bc858.bundle.js. This version behaves correctly even in the docker build.
@agubler Sorry for the confusion - it's not dojo's fault. But I really wonder how and why I got things mixed up that badly.

I'll remove the issue for dojo again and update the official results.

agubler commented

@krausest oh how funny! Thanks for working that out 😃

krausest commented

It's been more than two years since this issue was found. I'll move all implementations with 694 to non-keyed for the chrome 102 run.