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

Benchmark for WP directives #1

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Benchmark for WP directives #1

wants to merge 19 commits into from

Conversation

DAreRodz
Copy link
Owner

@DAreRodz DAreRodz commented Oct 3, 2022

What?

Add a benchmark to measure WP directives' performance. ⏱️

Why?

This PR aims to provide a tool for measuring the WP directives' performance while we iterate over their API and potential implementation. We can also compare WP directives with other frameworks or even different implementations (e.g., using useState, preact signals, etc.).

How?

To start, I've copied the directives from WordPress/gutenberg#44509, which is the experiment that contains the latest features at this moment (directives & components, processed during both parsing and client execution).

I had to modify the implementation to avoid using deepSignals (this), as it seems not to fully work as expected yet.

Also, I've implemented a missing wp-for directive to dynamically render a list of items inside a table (similar to Alpine's x-for directive). This directive still has room for improvement regarding performance.

Results

@luisherranz
Copy link

luisherranz commented Oct 5, 2022

it turns out that WP directives are not "keyed", i.e., keyed elements rearranged inside an array are not modified but their content. That would make WP directives not comparable to Alpine/Preact (¿?)

Could you please elaborate more on this? Preact is keyed, so we should be able to use their key mechanism. I've actually used it even in the static vnodes here.

So, I guess this key isn't working. Is that correct?

return list.map((item) => (
  <mainContext.Provider
    value={[{ ...context, [name]: item }, setContext]}
    key={item[key]}
  >
    {element.props.children}
  </mainContext.Provider>
));

@DAreRodz
Copy link
Owner Author

DAreRodz commented Oct 5, 2022

Yes, you are right; I was about to clarify that. The reason the WP directives framework ends in the "non-keyed" category is because of an issue with our implementation.

I've already replaced key with key.default, but that's not all; there are still nodes that are re-mounted even though their key doesn't change. I'm digging into that.

EDIT: I used the question marks (¿?) because I was confused as well, haha. 😅 🤷

@luisherranz
Copy link

Ok, thanks David!

@DAreRodz
Copy link
Owner Author

DAreRodz commented Oct 5, 2022

Uhm, it was that bug using key what was making deepSignal behave so slow. 🤔 The same issues appear as well after fixing it, so it looks like a problem with the wp-each directive implementation.

@DAreRodz
Copy link
Owner Author

DAreRodz commented Oct 5, 2022

Preliminary results

The following picture showcases WP directives' current performance results (at 01e565c) compared with Preact and Alpine. Our framework is expected to fall between both, but we can see that it performs poorly in some tasks, like "select row", and appears in last place.

The current wp-each implementation could cause this. As we need to extend the context for each child to include a different item, a new context is generated for each child every time the context is updated. We need to find an alternative solution to pass item down.

Screenshot 2022-10-05 at 18 16 19

@luisherranz
Copy link

there are still nodes that are re-mounted even though their key doesn't change

I've opened a bug in Preact's repo:

@luisherranz
Copy link

there are still nodes that are re-mounted even though their key doesn't change

The issue is fixed and should be available once Preact releases a new version:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants