Skip to content

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 27, 2024

The bench was a bit outdated. Moved from Chrome 117 -> 130

Data - https://krausest.github.io/js-framework-benchmark/2024/table_chrome_130.0.6723.58.html

Before
Screenshot 2024-10-27 at 15 21 12

After

Screenshot 2024-10-27 at 15 21 40

@ryansolid
Copy link
Member

Only comment is what versions of frameworks should we be using? Should we be using the faster "class" versions of React and Preact or should we use the slower "hooks" versions. We used to use Preact Classes and React Hooks as those at one point were the fastest implementation. But now React Classes are faster.

While I'm sure about zero people use React Classes, I wonder if we should penalize Preact the same way. Their Signals implementation is even faster than than Hooks (but still slower than Classes). Maybe I should ask twitter.

I will report back.

@WebReflection
Copy link

WebReflection commented Oct 28, 2024

@ryansolid i feel like that’s the same question we had to answer for closer to vanilla libraries, see uhtml: what should I do: use the common approach for events or use the better performing native delegate approach? As the bench is for using what people write when using this or that approach, like you suggested before for uhtml, you should use the pattern that suits most your offer. uhtml and lit got a special flag for “cheating the bench”, if any framework uses the non conventional way to perform it should get a special flag too, imho.

@birkskyum
Copy link
Member Author

birkskyum commented Oct 28, 2024

@WebReflection , i think it can be argued solid-js doesn't always use the common approach either, with the use of the createSelector function which does feel very niche, or maybe it's a coincidence, but it's consistently been too rigid to be particularly useful outside this context for me.

For context, the very first alpha of Svelte 5 had a selector just like solid, but it was removed early because it was considered too synthetic / unnecessary. I think that was a reasonable call to keep the api a tidy set of primitives, even though it to this day give Svelte slightly worse swap-rows performance.

When (hoping) solid ships a createProjection function, that can achieve the same as createSelector but in a generic way (described above conclusion in here), then I'd consider it completely fair game, and an improvement to the solid api.

@robbiespeed
Copy link

robbiespeed commented Oct 29, 2024

I'm not sure I agree about the createSelector part, if you were building a list with selection I think it would be the recommended option because it limits change propagation to only the affected rows, and doesn't really break any of the common solid idioms.

If you're going to pick the idiomatic option though, then I think solid-store should be chosen to represent solid. The data and it's updates all flow from outside the rows, so by nesting the setter of the label in the the row item itself it goes against the goal of only explicitly giving downstream consumers access to writes.

That being said I think you should pick the fastest option for all frameworks. If a certain pattern is causing a performance bottleneck, what should matter is: can it be improved using different patterns within the framework even if they aren't common at the time.

Edit:
Okay... slight adjustment to the above recommendation, because I just looked at the React docs for class components. I was not aware that the call out class components as a pitfall with the text "We recommend defining components as functions instead of classes" 4 separate times within the same page. That to me seems like a soft deprecation of class components. Which makes the line a bit trickier.
If one can reasonably expect that a feature is going to be dropped in the future then a benchmark using that feature shouldn't be chosen to represent the project. Certainly seems that way with react class components.

@ryansolid
Copy link
Member

Ok so the results were overwhelming in favor of hooks. I think for React this is the right implementation because the docs even suggest that classes are legacy. That being said I think Preact has a fair claim to say that classes are recommended given their docs. While the less popular option and not direct comparison with React, I think Preact Classes is probably what we should use.

@birkskyum if you can change Preact Hooks to Preact Classes I'd be happy to merge. Thanks for indulging me.

@birkskyum
Copy link
Member Author

@ryansolid excellent, it's Preact Classes now.

Screenshot 2024-10-29 at 21 26 58

@ryansolid
Copy link
Member

Alright LGTM

@davedbase davedbase merged commit 6273ad3 into solidjs:main Oct 30, 2024
@davedbase
Copy link
Member

Thanks everyone!

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