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

Interactivity API: Prevent calling proxifyContext with context proxies inside wp-context #65090

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

DAreRodz
Copy link
Contributor

@DAreRodz DAreRodz commented Sep 5, 2024

What?

Fixes a performance regression introduced in #62734, caused by proxifyContext being called over an already-proxified context repeatedly, thus increasing the number of proxies around the same context object every time a wp-context directive was rendered.
#62734

Why?

The ever-growing number of context proxies was causing the navigate() action to become exponentially slower.

The following pictures show the performance recording while clicking several times on the Next and Previous links in a Query block, before and after the changes containing the regression (#62734) are applied.

  1. trunk at c384bb6 (before)
    Screenshot 2024-09-05 at 11 47 22

  2. trunk at bd9ba41 (after)
    Screenshot 2024-09-05 at 11 46 08

How?

The fix prevents assigning to currentValue.current the result of calling proxifyContext( currentValue.current, ... ). This way, proxifyContext() never receives an already-proxified context as an argument when the wp-context directive is rendered multiple times.

Testing Instructions

  1. Visit a page with a Query block. The block should have the "Force page reload" option disabled.
  2. Repeatedly click on the Next and Previous links (over 20 times or more).
  3. Without this PR, the clicks should become noticeably slower over time. With these PR changes, everything should be good.

@DAreRodz DAreRodz added [Type] Bug An existing feature does not function as intended [Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity labels Sep 5, 2024
@DAreRodz DAreRodz self-assigned this Sep 5, 2024
@DAreRodz DAreRodz marked this pull request as ready for review September 5, 2024 13:45
Copy link

github-actions bot commented Sep 5, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: gziolo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

That makes perfect sense as proxifyContext already return the proxified version of currentValue.current 👍🏻

I'll give it a round of testing just to be on the safe side.

@vcanales, it's something that we will have to include in the upcoming Gutenberg plugin release.

@gziolo gziolo added [Type] Performance Related to performance efforts Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) and removed [Type] Bug An existing feature does not function as intended labels Sep 5, 2024
@gziolo gziolo modified the milestones: Gutenberg 19.3, Gutenberg 19.2 Sep 5, 2024
@DAreRodz DAreRodz merged commit e40447e into trunk Sep 5, 2024
71 checks passed
@DAreRodz DAreRodz deleted the fix/iapi-proxify-context-adding-multiple-proxies branch September 5, 2024 16:35
@gziolo
Copy link
Member

gziolo commented Sep 5, 2024

In my testing, the issue is still not fully resolved. After 30 seconds of testing Query block and clicking prev/next, the page becomes unusable:

Screenshot 2024-09-05 at 18 36 11

@gziolo
Copy link
Member

gziolo commented Sep 5, 2024

Ok, something must have gone wrong with the way I tested. I can now confirm that the issue is fully resolved and the performance graph is comparable to WordPress 6.6:

Screenshot 2024-09-05 at 18 56 44

DAreRodz added a commit to DAreRodz/woocommerce that referenced this pull request Sep 6, 2024
@DAreRodz DAreRodz changed the title Interactivity API: Prevent calling proxifyContext over an already-proxified context inside the wp-context directive Interactivity API: Prevent calling proxifyContext with context proxies inside wp-context Sep 9, 2024
vcanales pushed a commit that referenced this pull request Sep 11, 2024
…roxified context inside the `wp-context` directive (#65090)

* Do not store the proxified context inside  `currentValue.current`.

* Update changelog

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: gziolo <[email protected]>
cbravobernal pushed a commit that referenced this pull request Sep 11, 2024
* Core Data: Fix the 'query._fields' property check inside 'getEntityRecord' resolver (#65079)


Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: tyxla <[email protected]>

* Site Title, Post Title: Fix typography for blocks with `a` children (#64911)

* Apply styles for element + `a` child when present

* Inherit styles from parent element when present

* Add missing `-font-family` attribute selector

* Interactivity API: Prevent calling `proxifyContext` over an already-proxified context inside the `wp-context` directive (#65090)

* Do not store the proxified context inside  `currentValue.current`.

* Update changelog

Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: gziolo <[email protected]>

---------

Co-authored-by: George Mamadashvili <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: Rafael Gallani <[email protected]>
Co-authored-by: David Arenas <[email protected]>
Co-authored-by: DAreRodz <[email protected]>
Co-authored-by: gziolo <[email protected]>
dinhtungdu pushed a commit to woocommerce/woocommerce that referenced this pull request Sep 18, 2024
@kevin940726 kevin940726 removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Interactivity API API to add frontend interactivity to blocks. [Packages] Interactivity /packages/interactivity [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants