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

Cancel queued findWordsWithSubsequence jobs when mutating a buffer #44

Merged
merged 5 commits into from Nov 14, 2017

Conversation

Projects
None yet
3 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Nov 13, 2017

Fixes atom/atom#16153
Fixes atom/atom#16159
Fixes atom/language-shellscript#88

Background

After enabling the new Subsequence autocomplete provider by default in Atom, we observed a bad slow-down when holding down an alphanumeric key. This was happening because we would were calling findWordsWithSubsequence many times in rapid succession, interspersed with buffer changes. This was resulting in a large number of text buffer layers being created.

Solution

We have made three changes to fix this problem:

  1. When mutating a TextBuffer, we now cancel any findWordsWithSubsequence jobs that were initiated before the mutation and are still queued. Those calls will then resolve with null. This is ok because the result of those calls is no longer needed - a character has been typed since they were performed.

  2. We have optimized the internal TextBuffer::clip_position method so that in its cost now scales linearly with number of buffer layers. Previously it accidentally scaled exponentially with the number of layers because each layer would call clip_position twice on its underlying layer.

  3. We have placed a hard limit on the number of subsequence match variants that we consider when scoring a given match. This stops the findWordsWithSubsequence background thread from using an amount of time and memory that scales exponentially with the length of the query (the word fragment preceding the cursor in Atom).
    I have some ideas for algorithmic improvements to the scoring algorithm that will remove the exponential behavior entirely, but I think we can merge this PR as-is in order to fix the pressing problem in Atom.

@maxbrunsfeld maxbrunsfeld requested a review from nathansobo Nov 13, 2017

@Arcanemagus

This comment has been minimized.

Show comment
Hide comment
@Arcanemagus

Arcanemagus Nov 13, 2017

Does this also fix atom/language-shellscript#88? The Atom issues were marked as a duplicate of it, but the behavior is different.

Arcanemagus commented Nov 13, 2017

Does this also fix atom/language-shellscript#88? The Atom issues were marked as a duplicate of it, but the behavior is different.

));
);
text_buffer_wrapper->outstanding_workers.insert(worker);

This comment has been minimized.

@nathansobo

nathansobo Nov 13, 2017

Contributor

I know this is the only case where we have outstanding workers, but might it make sense to give this set and the cancellation method a specific name... to cancel outstanding subsequence searches?

@nathansobo

nathansobo Nov 13, 2017

Contributor

I know this is the only case where we have outstanding workers, but might it make sense to give this set and the cancellation method a specific name... to cancel outstanding subsequence searches?

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Nov 13, 2017

Contributor

I think that in the future, it needs to include other types of workers as well. For example, you can trigger the same problem by calling findAll(regex) many times rapidly, interspersed with buffer changes. We haven't made those calls cancellable yet. I'm not sure if we should do that as part of this PR, since currently there's nothing in Atom that would call that API on every keystroke. Maybe we should just go ahead and do it though.

@maxbrunsfeld

maxbrunsfeld Nov 13, 2017

Contributor

I think that in the future, it needs to include other types of workers as well. For example, you can trigger the same problem by calling findAll(regex) many times rapidly, interspersed with buffer changes. We haven't made those calls cancellable yet. I'm not sure if we should do that as part of this PR, since currently there's nothing in Atom that would call that API on every keystroke. Maybe we should just go ahead and do it though.

This comment has been minimized.

@nathansobo

nathansobo Nov 13, 2017

Contributor

Yeah, that makes sense. This set is for anything that should be cancelled on the next edit.

@nathansobo

nathansobo Nov 13, 2017

Contributor

Yeah, that makes sense. This set is for anything that should be cancelled on the next edit.

maxbrunsfeld added some commits Nov 13, 2017

Ensure that clip_position always scales linearly w/ the layer count
Signed-off-by: Justin Ratner <leroix08@gmail.com>
Avoid unnecessary position clipping in for_each_chunk_in_range
Signed-off-by: Justin Ratner <leroix08@gmail.com>
@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Nov 14, 2017

Contributor

Does this also fix atom/language-shellscript#88?

Yes, I believe so! Added to the description. Thanks @Arcanemagus.

Contributor

maxbrunsfeld commented Nov 14, 2017

Does this also fix atom/language-shellscript#88?

Yes, I believe so! Added to the description. Thanks @Arcanemagus.

@maxbrunsfeld maxbrunsfeld merged commit d3ab83e into master Nov 14, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-jr-fix-subsequence-slowness branch Nov 14, 2017

@rsese rsese referenced this pull request Sep 14, 2018

Closed

High CPU usage when fed a long string of digits #17725

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment