Core data: Performance: fix receive user permissions #64894
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
requested review from
nerrad,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
added
[Type] Performance
|
Let's add some essential documentation and unit tests for |
|
Size Change: +1.47 kB (+0.08%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
jsnajdr
reviewed
Aug 29, 2024
| } | ||
| } | ||
|
|
||
| dispatch.receiveUserPermissions( | ||
| receiveUserPermissionArgs |
There was a problem hiding this comment.
There should be Object.fromEntries( receiveUserPermissionArgs ), otherwise the reducer will store garbage into the userPermissions state.
tyxla
reviewed
Aug 29, 2024
| @@ -876,6 +876,10 @@ _Returns_ | |||
|
|
|||
| - `Object`: Action object. | |||
|
|
|||
| ### receiveUserPermissions | |||
There was a problem hiding this comment.
Just like receiveUserPermission, should this be ignored from documentation?
I'll add some inline docs and @ignore it.
Comment on lines
+526
to
+527
| ...state, | ||
| ...action.permissions, |
There was a problem hiding this comment.
Should we keep the existing permissions? I'd argue that we shouldn't, because if a user loses a permission in the meantime, it should be removed from the list of permissions.
Suggested change
| ...state, | |
| ...action.permissions, | |
| ...action.permissions, |
There was a problem hiding this comment.
I'm not sure I'm following here. Why would be dump the existing permissions from other resources? Also let's keep the logic the same as the singular form of the action in this PR, I don't want there to be any changes other than performance.
There was a problem hiding this comment.
Clearing the entire permissions state would be a mistake, because there are permissions for other resources.
The point is probably that if an OPTIONS request (or equivalent one) returns a header:
Allow: GET
then it means not only that there is a read: true permission, but that there is also update: false, create: false, delete: false. But we already take care of that, in the getUserPermissionsFromAllowHeader function.
There was a problem hiding this comment.
Ah, I see. Somehow I thought this retrieves them all at ince, which isn't the case, obviously.
There was a problem hiding this comment.
It retrieves all permissions at once for a query, but there are other queries or direct canUser calls, which all are stored in the same state, keyed by actions and resourceEntity.
|
I knew about the incomplete documentation! Was hoping someone verify the fix. |
Sure thing, working on it now. |
tyxla
approved these changes
Aug 30, 2024
There was a problem hiding this comment.
In my tests, the freeze was not 25s, but "nicely" over 10s:
With this branch, it gets down to 328ms, which is a great improvement 👍
Given that I'm on a very powerful machine, > 300 ms of unresponsiveness is still too much, but this PR is clearly an improvement.
As a follow-up we should consider splitting into smaller tasks, but that can be investigated in a separate PR.
Thanks for the fix @ellatrix 🚀
|
Wow, that's a long microtask 😅 Should we add an inline note in one of these |
Mamaduka
approved these changes
Aug 30, 2024
There was a problem hiding this comment.
Thank you, @ellatrix!
I couldn't spot any regressions, and the inserter open time visibly improved.
The performance test results also look good; no regressions here. See https://github.com/WordPress/gutenberg/actions/runs/10628391881.
P.S. I'll follow up with a similar patch for the getEntityRecrod resolver. It could improve performance a little when a page has many controlled blocks.
|
Omg, I'm so sorry, I forgot the props... Sorry I've been away from making GB PRs for a bit and forgot about this. 🙁 |
|
We should really automate that. I think the only thing that would work is have a "merge" label and let a bot merge the PRs. |
|
No worries, @ellatrix! |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes 25s of lag when opening the inserter when there are 3000 reusable blocks on the site. This is caused by #64504.
Why?
How?
The problem is that each user permission is received separately. This can be solved by creating a new action the receive permission in one batch.
Testing Instructions
wp eval 'for ($i = 1; $i <= 3000; $i++) { wp_insert_post(["post_title" => "Reusable Block " . $i, "post_content" => "<!-- wp:paragraph --><p>This is block " . $i . "</p><!-- /wp:paragraph -->", "post_status" => "publish", "post_type" => "wp_block"]); }'Testing Instructions for Keyboard
Screenshots or screencast