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

added react-hookstate and preact-hookstate frameworks #693

Closed
wants to merge 3 commits into from

Conversation

avkonst
Copy link

@avkonst avkonst commented Feb 12, 2020

Please merge these two frameworks. Hookstate is a competitor of Redux, Mobx. And it does really well in the performance benchmark. Target per single row updates are extremely fast with Hookstate in particular. Performance results from my experiments are discussed here: avkonst/hookstate#14

@krausest
Copy link
Owner

Looks very interesting. I‘ll try to merge the next few days. Did you check whether your implementation is actually keyed (npm run isKeyed in webdriver-ts)? I’m asking because your link says swap rows is faster than vanillajs which would be cool, but often turns out to be a non-keyed update...

@avkonst
Copy link
Author

avkonst commented Feb 12, 2020

I have cloned react-hooks from keyed one. Hookstate.useStateLink is a super-charged React.useState. I have done as close as possible handling of dispatch events as react-hooks and react-redux-hooks do. Hookstate does smart tracking of used/rendered states and rerenders only the affected areas. So, I do not know why react-hookstate would be categorised differently to react-hooks, for example. Here is what I see in the output of npm run isKeyed -- --framework hookstate:

react-hookstate-v16.8.6 + 1.5.2-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

Why does it fail keyed test for swap rows?

@avkonst
Copy link
Author

avkonst commented Feb 12, 2020

I have found using the check tool that it expects 1TR to be added and 1TR to be removed. It is strange it does not accept in place updates. I have found a workaround for this, checking performance now.

@avkonst
Copy link
Author

avkonst commented Feb 12, 2020

It passes the check when I disable state tracking in Hookstate. It forces the TR to be replaced everytime it's state is changed. Why do you expect for swap rows to replace the whole TR but not it's text content and danger class?

@avkonst
Copy link
Author

avkonst commented Feb 12, 2020

OK. I have found the difference between keyed vs non-keyed in react frameworks. I will have a look if I can do both for Hookstate.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Ok. fixed now check for isKeyed. It turned out I needed to add key property for tr tag. When id of the key property changes, React replaces this component, hence it satisfies the check. This slowed down some benchmarks, but still looking great for hookstate, in particular it is 10x times faster than redux on per row updates. I have also added non-keyed variant of the react-hookstate.

image

}

const globalState = createStateLink({});
let selectedState = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it look nice if we put selectedState in globalState?
I mean I think the pro of your lib is "immutable updates" (correct me if I'm wrong), and having mutable state here seems unfortunate. I assume the change wouldn't affect the performance.

Copy link
Author

@avkonst avkonst Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state can be mutated only via StateLink.set() API. Not sure if it satisfies the criteria of immutability. Regarding the selected state. It is actually within the global state - each row in the state has got field selected. This selectedState variable is a fast index variable / cache. It points to the StateLink from globalState, which had selected field set to true last time. This allows to locate relevant/selected StateLink in order to do unselect more efficiently, i.e. without searching for one (although, I think searching over 1000 rows in the list would not be much a problem. It would be worse with larger list).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immutability

I might have misunderstood from your original implementation. So, your lib is more like MobX. (Which implies React.memo won't work as expected in some cases. Sorry, this discussion is nothing to do with benchmark. Let's do it in the other thread.)

fast index variable / cache

That sounds unrealistic in the typical React coding pattern. You might want at least to put it in Context/useRef. The benchmark shouldn't be too optimized.

Copy link
Author

@avkonst avkonst Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it in the useRef, makes no difference for performance. But if you have got strong feelings about it, I can move it back to useRef. Alternatively, I can do search for unselecting unknown selected. It should not change much too. I will keep it as it is, unless you speak up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it in the useRef, makes no difference for performance.

Yeah, that was my expectation.
So, in your real app, do you use useRef for the cache?

I can do search for unselecting unknown selected.

You mean without let selectedState or useRef? I'm interested in it.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Copy from the original discussion avkonst/hookstate#14 (comment):

keyed/react-hookstate does effectively the following: 'delete TR at 1st and 998th position' and 'insert new TR at 1st and 998th position'.

non-keyed/react-hookstate does the following: 'set TD text for the label and id and TR class for 'selected' for the 1st row copying state from the 998th' and vice versa.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

swap rows confirmed incredibly fast performance of Hookstate on per field update originally demonstrated here: https://hookstate.netlify.com/performance-demo-large-table

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

there was also a success story where Hookstate helped with very frequent 'per field' updates in the large array data set. Read more here: https://praisethemoon.org/hookstate-how-one-small-react-library-saved-moonpiano/ there is a video at the end demonstrating fast rerendering performance of React + Hookstate.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Here is the latest results from swap rows benchmark:

image

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

The difference between keyed/react-hookstate and keyed/react (or keyed/react-hooks) is because, the first rerenders only 2 rows (calling Row component only, which replaces the TR tag entirely). The second rerenders the whole table (calling Rows component), although taking 998 rows from memo/cache while looping.

@ryansolid
Copy link
Contributor

ryansolid commented Feb 13, 2020

That is interesting. @krausest @leeoniya @localvoid might find this of interest.

  1. Your select row is fast because you are pushing selection state on to the items. While I don't believe it is strictly forbidden (a few libraries do it) it isn't equivalent to the other React implementations. They purposefully do it the other way since its what is being tested.. ie not to muddle the model state. The way this is currently implemented tests the same thing as the partial update test rather than something different. I'd consider changing this if you are interested in doing an equivalent comparison.

  2. Swap Row is interesting. The other React libraries grab all Rows from cache. Technically they move the 2 DOM elements but Row Component is re-run 0 times since it is unchanged and memoized. By changing the list it just runs the diff and DOM reconciler. Your implementation re-creates the row without triggering the reconciler as the map function doesn't trigger. So it avoids React's naive reconciliation making it basically the cost of creating and attaching a few DOM nodes. It's a cool trick, but in no way intended. It basically bypasses the test. But points for creativity, I haven't seen anyone take this approach with React before.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

If I drop hookstate scoped state (point 1 and 2), it will make it the "equivalent ' to react hooks and will not justify why hookstate exists. I do not want another redux, I want high performance rerendering followed by state changes. If it is not allowed I do not see the point in benchmarking various libs as they are forced to hit react overhead.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

I would not restrict selected to be a part of an item. Redux and other libs could use the same approach but it would not improve anything for them as they will go through the map rows anyway

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

2. creating and attaching a few DOM nodes. It's a cool trick, but in no way intended

Well vanilla js does exactly this, why can not I do the same in my framework?

@ryansolid
Copy link
Contributor

ryansolid commented Feb 13, 2020

React MobX and React Easy State could do the same thing and benefit. Basically any of the reactive React libs have this ability as well.

1 has come up a number of times before so its a known thing. To be fair select row is the most abused test in this benchmark so it's not surprising. In some cases, it's just how the library works. Sinuous's template API which is vital to its performance can't be done any other way but assigning to the item. And there are many other more questionable approaches there. I get how this is basically the whole point of the library to delegate changes downward so it can essentially forceUpdate in the listening components instead of doing some top-down immutable thing. So I can see the argument for this being fair game. At a certain point, many libraries might just go this way since for reactive libraries and even some not this is just the more performant way to go. I feel it goes against the spirit of that test, but it's a lesser evil than some of the other solutions here.

2 is abusing a trick. If you were writing the rendering library (like React) you wouldn't consider this approach as it is bypassing something you know your library has to do anyway. Like destroying and creating nodes every time you move something might not be acceptable. If you say had to sort a list alphabetically you might end up being slower because you needed to recreate the whole list. Or maybe you are using WebComponents or DOM nodes that contain state. It sort of abuses the Keyed mentality.

Well vanilla js does exactly this, why can not I do the same in my framework?

Vanilla doesn't destroy the nodes. It reuses them. Ie it moves the same nodes. A way to avoid reconciliation here while keeping node references is if the render was essentially a proxy (see Mikado). However, that has similar issues when the reordering is more complex than a swap having terrible scalability without the ability to batch and apply several transformations. Some libraries like Crui are looking into ways to do this in a more scalable way but they don't have 100% coverage and can only optimize for small set of known transformations currently.

Vanilla also holds a DOM reference to the selected TR to do selection but any data-driven implementor would recognize that basically breaks the mental model(ie.. it can not be re-rendered purely from state at any point). So the Vanilla does it why can't I is a slippery position. Don't get me wrong I think that ultimately people will take the fastest methods so all of these rules and posturings are pointless. However, there is value in how a library is presented for comparison beyond the performance.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Sorry, I do not understand the point about abusing the trick. The test is to swap two rows in the state and expect correct rendering. If it was different requirement like sorting, it would trigger whole list rerender by hookstate correctly, but there was no a requirement to sort items.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Moving selected marker out of the item state is doable, if you say it is not part of a state. I can just keep selected id in a separate state variable. I can do it without any performance impact. But do not see why the approach I taken is wrong.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Btw, this benxhmark is missing very representative use case: editing of a single field, like a text box in a large form. Swap rows for hookstate is like editing 2 fields. Other state management libs also edit two rows, but are not smart enough to detect this hence whole list rerender.

@ryansolid
Copy link
Contributor

ryansolid commented Feb 13, 2020

Update every 10th row(Partial Update) is similar to that case where more optimal approaches won't trigger reconciliation. The way you do select rows also is this scenario.

Swap Rows is the only test in the benchmark that actually tests list reconciliation with changed positions (technically replace, remove row and append do too but nothing has moved). There were some attempts to add more interesting move tests but it proved difficult to make them fair(predictable cost) while not making them optimizable for since they were predictable. Just left things at sort.

Understanding how your library works I see the reason to leave select row as it is. That test was born out of an MVC time where you'd never store your selection state on the global models or store. But that isn't how things are always done today, with some putting all their state in global state containers. So even if it isn't equivalent it seems it could be idiomatic for your approach. Any change you made would still basically work the same I imagine.

Keyed Swap Rows destroying and recreating rows does not seem idiomatic. It doesn't scale on node size or number of nodes. It doesn't swap the rows in a keyed sense.. ie TR rows that correspond with data are moved in the DOM preserving DOM state. It just happens to be faster than React's naive list sorting on this particular scenario. I think we should change isKeyed test to detect this and fail.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Update every 10th row(Partial Update) is similar to that case where more optimal approaches won't trigger reconciliation.

This is only partially in the same bucket. This test requires Hookstate to trigger rerender for 100 rows (but not only 1, like in the typical scenario when a user edits a form field). Updates for 100 rows individually are only 20-30% faster in react than updating the whole 100 rows table. If it was only 1 field update, the difference in performance between react-hookstate and react-hooks and others would be in similar magnitude as swap rows currently.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

Understanding how your library works I see the reason to leave select row as it is.

Thanks for understanding.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

It doesn't swap the rows in a keyed sense.. ie TR rows that correspond with data are moved in the DOM preserving DOM state.

Not every framework will be able to move the existing dom elements without destroying / recreating.

@avkonst
Copy link
Author

avkonst commented Feb 13, 2020

destroying and recreating rows does not seem idiomatic. It doesn't scale on node size or number of nodes

I will be happy to see the new benchmark where more aggressive shuffling is done (i.e. sort by label?). This will be representative for scalability and can be verified. Sorry, swap rows does not reflect this "ideomatic" intent.

@avkonst
Copy link
Author

avkonst commented Feb 14, 2020

I think that ultimately people will take the fastest methods so all of these rules and posturings are pointless. However, there is value in how a library is presented for comparison beyond the performance.

So, shall we approve the PR?

@leeoniya
Copy link
Contributor

I will be happy to see the new benchmark where more aggressive shuffling is done

the labels are random, so the results would not be deterministic between runs :(

i think a lot of frameworks will do poorly enough on sorting 1000 elements, as to spell an untimely death for krausest's laptop.

@avkonst
Copy link
Author

avkonst commented Feb 14, 2020

I will be happy to see the new benchmark where more aggressive shuffling is done

the labels are random, so the results would not be deterministic between runs :(

i think a lot of frameworks will do poorly enough on sorting 1000 elements, as to spell an untimely death for krausest's laptop.

sorting was only an example. other more aggressive shuffling could be 'reverse', '1,999,2,998,3,997, etc..' The point is not about sorting but about enforcing "idiomatic intent" by the test.

@leeoniya
Copy link
Contributor

The point is not about sorting but about enforcing "idiomatic intent" by the test.

right, i'm 100% for having a test like this.

@avkonst
Copy link
Author

avkonst commented Feb 14, 2020

It doesn't swap the rows in a keyed sense.. ie TR rows that correspond with data are moved in the DOM preserving DOM state. It just happens to be faster than React's naive list sorting on this particular scenario. I think we should change isKeyed test to detect this and fail.

Few question here:
So, the expectation is 2 TR rows are swapped, but not destroyed/recreated? If my framework could do exactly this, would it pass your criteria, which you are proposing to verify in the improved isKeyed test?

@leeoniya
Copy link
Contributor

So, the expectation is 2 TR rows are swapped, but not destroyed/recreated?

yes. the current test is a bit rudimentary in that it does not actually test that a swap happened. MutationObserver can report a remove/insert even when the elements are adjacent and detached/reinserted in one op:

https://jsfiddle.net/wfa5v71s/

this test should probably be improved and may reveal some existing inconsistencies in framework behavior. the original intent of the test was to quickly detect keyed vs non-keyed behavior.

@avkonst
Copy link
Author

avkonst commented Feb 14, 2020

So, the expectation is 2 TR rows are swapped, but not destroyed/recreated?

yes. the current test is a bit rudimentary in that it does not actually test that a swap happened. MutationObserver can report a remove/insert even when the elements are adjacent and detached/reinserted in one op:

https://jsfiddle.net/wfa5v71s/

this test should probably be improved and may reveal some existing inconsistencies in framework behavior. the original intent of the test was to quickly detect keyed vs non-keyed behavior.

So, if yes, the expectation is that a framework does it in any way it can do it (assuming it is coded like it would be in a real app)? For example, Hookstate has got existing StateMemo routine which behaves like React.memo, but the input is a state and the memoised state is anything (can be DOM, why not? just never though about using StateMemo to memo DOM). It would swap rows without creating/destroying TRs (will verify it next week, not 100% sure currently). Will it pass the criteria?

@avkonst
Copy link
Author

avkonst commented Feb 14, 2020

Hookstate has got existing StateMemo routine which behaves like React.memo, but the input is a state and the memoised state is anything (can be DOM, why not? just never though about using StateMemo to memo DOM). It would swap rows without creating/destroying TRs (will verify it next week, not 100% sure currently).

OK. I must admit that it is impossible to move two rows by Hookstate without asking React to do it, or without deleting/creating them. So, for me the only way to keep the current isKeyed check happy is to destroy/create rows.

If it is not acceptable, I will need to drop off scoped states and make the implementation really equal to react-hooks (this will make the performance equal to react-hooks - confirmed as I started with it). In this case the benchmark will not give much value for me, because it will not highlight the strongest benefit of Hookstate - fast in place per row updates (as I mentioned above there is no such a test to cover this scenario).

So, @krausest , please let me know your verdict. As far as the current isKeyed check concerned - it is passing.

@krausest
Copy link
Owner

In the true sense of a keyed update I think it‘s a requirement that the dom nodes are actually moved and not recreated (keyed means dom nodes and data list items are linked). I‘ll check if the isKeyed test can detect this and I‘m pretty excited what it‘ll report for the other frameworks. I hope to report back soon. (Please leave the PR open)

@krausest krausest mentioned this pull request Feb 14, 2020
12 tasks
@krausest
Copy link
Owner

I opened #694 for the swap rows test.

@krausest
Copy link
Owner

FYI: I'm on holidays the next few days and will leave the PR open and decide how to proceed on when I'm back.

@krausest
Copy link
Owner

krausest commented Apr 5, 2020

In the mean time I've flagged all keyed implementations that do not properly handle swap rows.
I think at some point in time I'll move those implementations to the non-keyed section.
Is there a chance to implement swap rows as a keyed operation for hookstate?

@avkonst
Copy link
Author

avkonst commented Apr 5, 2020

Yes, there is a way to do it. It will not be much more different than plain react hooks, because of the constraint on swap rows mechanics you placed. I will probably do 2 implementations keyed and non-keyed. Apparently Hookstate will be the first framework for React to add non-keyed implementation.
Overall, your benchmark needs new test: update of a single field (not 10% of rows, but just one). It is very frequent and common scenario - users edit one field at a time when deal with settings, forms, etc. This is the case where Hookstate performance is nearly equal to vanilla JS outperforming Redux and alike by magnitude.

@krausest
Copy link
Owner

krausest commented Jul 6, 2020

I'm closing this PR due to a lack of updates.

@krausest krausest closed this Jul 6, 2020
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

Successfully merging this pull request may close these issues.

5 participants