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

Defer bracket pair document parsing until it's actually used #163958

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Oct 18, 2022

Part of #161622

This change queues parsing of the document so that it does not block editor input latency. This is done via setImmediate such that it is called before any timers, the only downside to this is that changing a character adjacent to a bracket may appear white for a single frame.

This is a good trade off imo to save what I measured sometimes to be > 10% of the keypress event's runtime. The amount of time spent is quite variable, not sure why.

@Tyriar Tyriar added this to the October 2022 milestone Oct 18, 2022
@Tyriar Tyriar requested review from hediet and alexdima October 18, 2022 18:24
@Tyriar Tyriar self-assigned this Oct 18, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Oct 19, 2022

@alexdima can I not use set/clearImmediate in monaco?

Error: src/vs/editor/common/model/bracketPairsTextModelPart/bracketPairsTree/bracketPairsTree.ts(76,12): error TS233[9](https://github.com/microsoft/vscode/actions/runs/3276846488/jobs/5393380511#step:10:10): Property 'clearImmediate' does not exist on type 'Window & typeof globalThis'.
Error: src/vs/editor/common/model/bracketPairsTextModelPart/bracketPairsTree/bracketPairsTree.ts(123,31): error TS2339: Property 'setImmediate' does not exist on type 'Window & typeof globalThis'.

@alexdima
Copy link
Member

can I not use set/clearImmediate in monaco?

Unfortunately setImmediate is out of the standards track and not available on the web. AFAIK it is deprecated and possibly only available in Electron/node.js

@Tyriar
Copy link
Member Author

Tyriar commented Oct 19, 2022

@alexdima good to know, will switch to timeout

@Tyriar
Copy link
Member Author

Tyriar commented Oct 19, 2022

This is where it happens now:

image

Probably better than immediate actually as immediate was happening before the animation frame

@Tyriar Tyriar changed the title Defer bracket pair document parsing using setImmediate Defer bracket pair document parsing using setTimeout Oct 19, 2022
@Tyriar Tyriar changed the title Defer bracket pair document parsing using setTimeout Defer bracket pair document parsing until it's actually used Oct 24, 2022
@Tyriar Tyriar requested a review from alexdima October 24, 2022 12:26
@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

@hediet ready for a review, this has the changes @alexdima and I discussed which should ensure the state is correct when public APIs are called by flushing the queue at the last moment.

@Tyriar Tyriar enabled auto-merge October 24, 2022 12:42
@Tyriar Tyriar requested a review from rebornix October 24, 2022 19:50
@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

Adding @rebornix so I can get this in before endgame

@alexdima
Copy link
Member

@Tyriar I would feel most comfortable if we can also get @hediet's review here, as he authored/owns that code.

@alexdima alexdima removed their request for review October 24, 2022 20:01
@alexdima alexdima dismissed their stale review October 24, 2022 20:01

out-of-date

@Tyriar Tyriar merged commit 590729f into main Oct 24, 2022
@Tyriar Tyriar deleted the tyriar/161622_bracket_queue branch October 24, 2022 20:01
@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

@alexdima oh auto merge was enabled while I was typing. My thinking was he was already keen on the idea, just that it caused issues with the timeout. So merging now would make it in for testing and he could comment/revert if there is a problem with it.

@Tyriar
Copy link
Member Author

Tyriar commented Oct 24, 2022

I pinged @hediet about it to make sure he checks it out

@hediet
Copy link
Member

hediet commented Oct 25, 2022

The incremental bracket pair parser expects that the given edits precisely describe the changes between the state of the text model it saw the last time and the current state of the text model.
Queuing the changes and then processing them in the order they have been queued breaks this assumption, as the parser only sees the final state of the text model when processing these intermediate changes.

Imagine you start with this document:


}

Now the user

  • (1) adds { at line 1, column 1
  • (2) adds \n at line 1, column 1
  • (3) adds x at line 3, column 2

This translates to these edits:

  • (1) [{ start: (1:1), end: (1:1), newLength: (0:1) } ]
  • (2) [{ start: (1:1), end: (1:1), newLength: (1:0) }]
  • (3) [{ start: (3:2), end: (3:2), newLength: (0:1) }]

The state after (1) is:

{
}

The state after (2) is:


{
}

The state after (3) is:


{
}x

However, when the parser processes (1), it already sees the state (3).
The parser tries to reparse the text node at that position (line 1, column 1 to line 1, column 2) and still finds an empty line - thus the bracket tree is not touched.
Then it processes the \n and x insert changes and again does not touch the brackets 🐛


What would work though is to combine the changes, as we discussed some time ago (for precisely this problem).

In that case, you would only issue this edit:

  • (1) [{ start: (1:1), end: (1:1), newLength: (1:1) }, { start: (2:2), end: (2:2), newLength: (0:1) }]

This would not only be correct, but also more performant, as the parser only needs to do one pass.

However, combining changes is not entirely trivial: You have to describe later changes in terms of the initial situation. This should be doable in linear time though.

Tyriar added a commit that referenced this pull request Oct 25, 2022
…et_queue"

This reverts commit 590729f, reversing
changes made to 867f456.
Tyriar added a commit that referenced this pull request Oct 25, 2022
Revert "Merge pull request #163958 from microsoft/tyriar/161622_brack…
formigoni pushed a commit to formigoni/vscode that referenced this pull request Oct 27, 2022
…ket_queue

Defer bracket pair document parsing until it's actually used
formigoni pushed a commit to formigoni/vscode that referenced this pull request Oct 27, 2022
…622_bracket_queue"

This reverts commit 590729f, reversing
changes made to 867f456.
formigoni pushed a commit to formigoni/vscode that referenced this pull request Oct 27, 2022
Revert "Merge pull request microsoft#163958 from microsoft/tyriar/161622_brack…
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants