Optimize getClientIdsOfDescendants and getClientIdsWithDescendants selectors.
#40054
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
ZebulanStanphill
added
[Type] Performance
ZebulanStanphill
requested review from
ellatrix,
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
|
Size Change: +8 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
|
I'm not sure if the performance tests are the ideal way to see the performance of individual functions. I'd consider that more for a larger refactoring where a benchmark isn't possible. For this, the scope is so small that it might be better to benchmark the individual functions, passing in some dummy data and timing how long the functions took (averaged over multiple runs). It could be a fairly homebrew benchmark, just using something like |
|
Thanks for following up @ZebulanStanphill In my reading of codehealth. the impact of this change on the "focus" metric was about @talldan I'm also not sure that checking the performance of the function directly will give any clue here because there's so many factors (number of times it's called, the arguments, the pre-rendering... ) that can impact the measure. Personally, given that the initial numbers of the "focus" metric in the performance tests of this PR are "promising", I think we should try it. Merge it and monitor the metric over the next commits in trunk. |
youknowriad
approved these changes
Apr 6, 2022
ZebulanStanphill
deleted the
update/optimize-descendant-client-ids-selectors
branch
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?
This PR (hopefully) optimizes the performance of the
getClientIdsOfDescendantsandgetClientIdsWithDescendantsselectors, to (hopefully) counteract any extra load caused by #39985.This PR also revises the description of
getClientIdsOfDescendantsto properly describe cases where multiple ids are passed to it.Why?
#39985 may have slowed them down a bit.
How?
By using for-of loops, we can avoid creating a bunch of intermediary arrays, which should presumably speed things up a bit.
Testing Instructions
Check the "Performances Tests" in the PR checks, I guess? Not sure how reliable that is, though.