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 s2 #857

Merged
merged 4 commits into from
Feb 28, 2021
Merged

add s2 #857

merged 4 commits into from
Feb 28, 2021

Conversation

gr0uch
Copy link
Contributor

@gr0uch gr0uch commented Feb 18, 2021

@gr0uch
Copy link
Contributor Author

gr0uch commented Feb 18, 2021

this is my second contribution to the benchmarks :)

@ryansolid
Copy link
Contributor

This one falls under #800. But it is also non-keyed so maybe no one cares.

@gr0uch
Copy link
Contributor Author

gr0uch commented Feb 18, 2021

This one falls under #800. But it is also non-keyed so maybe no one cares.

Seems that is correct, view state is on the model. But I am actually not sure if this qualifies as keyed or non-keyed, since it internally uses a WeakMap to associate a model to a DOM node, so actually one could mess with the DOM externally and this would continue to work as expected.

@ryansolid
Copy link
Contributor

That sounds like keyed then. If the data stays attached to the same DOM node for its lifetime. There is an isKeyed test.

@gr0uch
Copy link
Contributor Author

gr0uch commented Feb 18, 2021

@ryansolid thanks for the advice!

I did run the isKeyed test with npm run isKeyed -- --framework s2 and got:

s2-vinvalid (no package-lock)-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

so it is indeed keyed.

@krausest krausest added the merging started merging started (no more updates please) label Feb 28, 2021
@krausest krausest merged commit fb2b7b7 into krausest:master Feb 28, 2021
@krausest krausest removed the merging started merging started (no more updates please) label Feb 28, 2021
@krausest
Copy link
Owner

Results are updated:

Nice to see that it turned out to be keyed. Performance looks pretty good!

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.

3 participants