Performance: Avoid string-allocation on keypress with inputRule #47094
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
|
Size Change: +3 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7e8990f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4125347319
|
dmsnell
force-pushed
the
perf/avoid-string-slicing-on-keypress
branch
3 times, most recently
from
f56a7c7
to
6310b88
Compare
dmsnell
force-pushed
the
perf/avoid-string-slicing-on-keypress
branch
3 times, most recently
from
0ad2960
to
1e96652
Compare
|
@ellatrix I should have left this small and to-the-point before, as I managed to introduce a bug the way I originally formulated it. this smaller patch is passing all the tests and still covers what I cared about: the string allocations. Unlikely that we'll be able to measure the impact here on this without going into far more detail profiling than I currently know how to do. It should be ready for actual pre-merge review though. |
dmsnell
added a commit
that referenced
this pull request
Feb 8, 2023
While checking for inline code snippets surrounded by the backtick we have been allocating new substrings from input strings when looking for those characters, but we don't need to do this. In this patch we're directly checking for the character on the input string using string indexing and then passing the `position` parameter to the `lastIndexOf` function so that it is able to scan the original input string without copying it. The performance impact of this change should be small and hard to measure, but it should reduce pressure on the garbage collector and be worthwhile even without clear measured results.
dmsnell
force-pushed
the
perf/avoid-string-slicing-on-keypress
branch
from
1e96652
to
7e8990f
Compare
gziolo
added
[Type] Performance
gziolo
reviewed
Feb 9, 2023
|
|
||
| // Quick check the text for the necessary character. | ||
| if ( characterBefore !== BACKTICK ) { | ||
| return value; | ||
| } | ||
|
|
||
| const textBefore = text.slice( 0, start - 1 ); | ||
| const indexBefore = textBefore.lastIndexOf( BACKTICK ); | ||
| if ( start - 2 < 0 ) { |
There was a problem hiding this comment.
When start is 0 then characterBefore will always be undefined. When start is 1 then this will also bail out so we don't care so much about characterBefore. Maybe this check should run before line 23?
There was a problem hiding this comment.
I don't understand what is happening here.
There was a problem hiding this comment.
my purpose here is to minimize the change to leave it as small and focused as possible. what's happening is exactly what @gziolo noted. the check ensures that we don't introduce a regression by moving to lastIndexOf, which treats its position parameter as 0 if it's negative. without the - 2 check we would conflate backticks at 0 with no previous backticks.
There was a problem hiding this comment.
Would be good to comment inline
gziolo
approved these changes
Feb 9, 2023
There was a problem hiding this comment.
Code changes look good. It tests well when compared with trunk. I was able to reproduce the same issue covered in the description in both branches.
Unrelated to this PR. I also noticed in both branches that the editor handles only the second backtick. It doesn't handle the case when you go back and add a matching backtick:
backticks.mov
So that is also consistent in this PR.
ellatrix
reviewed
Feb 10, 2023
| @@ -20,16 +20,18 @@ export const code = { | |||
| __unstableInputRule( value ) { | |||
| const BACKTICK = '`'; | |||
| const { start, text } = value; | |||
| const characterBefore = text.slice( start - 1, start ); | |||
| const characterBefore = text[ start - 1 ]; | |||
There was a problem hiding this comment.
Maybe we should add characterBefore as one of the parameters of __unstableInputRule because almost all rules will need this.
Maybe even better yet, associate the callback with a trigger character so we can built this in? There's a reason why this API is not stable 😅
There was a problem hiding this comment.
maybe, but do we know this is the only use-case? apart from inline code I don't know what the purpose is here. would we ever want to run this on triple-backtick code fences?
I guess at some point most replacements would have a stable trigger character, unless we wanted to re-use code for something like mentions.
in any case text[ start - 1 ] is essentially free so I don't see it causing harm to leave in before we have a good feeling for what this API should end up looking like.
While checking for inline code snippets surrounded by the backtick we have been allocating new substrings from input strings when looking for those characters, but we don't need to do this. In this patch we're directly checking for the character on the input string using string indexing and then passing the `position` parameter to the `lastIndexOf` function so that it is able to scan the original input string without copying it. The performance impact of this change should be small and hard to measure, but it should reduce pressure on the garbage collector and be worthwhile even without clear measured results.
dmsnell
force-pushed
the
perf/avoid-string-slicing-on-keypress
branch
from
7e8990f
to
ca90b31
Compare
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?
Introduce subtle performance refactor.
Why?
Every bit of code that runs on every keypress is in the critical hot path. If we can avoid GC pressure, memory allocations, or CPU cycles, we should examine that to guard the responsiveness of the editor for someone while typing.
How?
While checking for inline code snippets surrounded by the backtick we have been allocating new substrings from input strings when looking for those characters, but we don't need to do this.
In this patch we're directly checking for the character on the input string using string indexing and then passing the
positionparameter to thelastIndexOffunction so that it is able to scan the original input string without copying it.The performance impact of this change should be small and hard to measure, but it should reduce pressure on the garbage collector and be worthwhile even without clear measured results.
Testing Instructions
Verify that inline code snippets are still functioning the way we expect them to. Check at the beginning of a line, or at the end of one.
Try by adding backticks around existing text.
Note that some peculiarities of the inline code behavior is already in
trunk, for example, when entering a new backtick when others exist already in a paragraph. The following video was recorded fromtrunkto show that this isn't a new issue in this PR.Screen.Recording.2023-01-11.at.3.56.07.PM.mov