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

expose only the positions of those brackets colored by native bracket pair colorization #131062

Open
hediet opened this issue Aug 18, 2021 · 9 comments
Assignees
Labels
bracket-pair-colorization feature-request Request for new features or functionality
Milestone

Comments

@hediet
Copy link
Member

hediet commented Aug 18, 2021

Question: I know exposing tokens to extensions is a headache, but is it now possible and non-headache to expose only the positions of those brackets colored by native bracket pair colorization?
like this:
{type: "{" | "}" | "(" | ")" | "[" | "]"; line: number; column: number; }[]
or like this:
{type: "{" | "}" | "(" | ")" | "[" | "]"; globalStringIndex: number; }[]

No tree structure needed, just a simple array of positions. It will be super huge speed upgrade for Blockman (already 19k installs).

Originally posted by @leodevbro in #128465 (comment)

@hediet hediet self-assigned this Aug 18, 2021
@hediet hediet added the feature-request Request for new features or functionality label Aug 18, 2021
@hediet hediet added this to the Backlog milestone Aug 18, 2021
@leodevbro
Copy link

Oh, thanks so much for posting this feature request.

@hediet
Copy link
Member Author

hediet commented Aug 18, 2021

Syncing over all bracket positions might be headache.

However, I can imagine an async API that you can ask to query bracket pairs in a given range.
It could be limited to return a certain amount of bracket pairs (e.g. 1k).
I think bracket pair positions can also be encoded very efficient in some UInt32Array, so there is not much overhead (5 values per pair, so at most 20kb).

We don't know the globalStringIndex (only compute line/column for faster queries).
That's roughly what I have in mind:

/**
 * Returns undefined if bracket pair information are not available (e.g. because the feature is disabled or the document is too large).
 * Otherwise, returns a list of bracket pairs whose range intersects the given range.
 */
async function provideBracketPairs(model: TextModel, range: Range): Promise<BracketPairs | undefined>;

interface BracketPairs {
    pairs: BracketPair[];
    truncated: boolean; // Indicates whether there are more positions that are not included for performance reasons
    /**
     * Indicates if bracket pairs changed in the meantime.
     */
    inaccurate: boolean;
}

interface BracketPair {
    openRange: Range;
    closeRange: Range;
}

Would that be helpful? I don't know all the implications of this async API for your usecase.

@leodevbro
Copy link

If that approach is significantly faster than the Bracket Pair Colorizer 2 approach (Which is also async, and it's implemented in Blockman), then I guess it's worth making.

Just one note:
bracket number limit e.g. 1k is maybe fine, well, probably we should maximize this limit as big as possible, like 10k maybe.

Well, I guess most of real world code files does not have more than 3k brackets even if they have 10 thousand lines.
But probably 10k (or more) bracket limit would be good just in case.

One more note:
Whatever the limit, 1k, 5k or 10k, I think Blockman cannot work with queried brackets of given range, it needs to know the entire file brackets, because the rendered blocks of current viewport window often can depend on the brackets which are located at the last line of the file or at the first line of the file, or both.

Well, for this one, I guess there is simple solution. If we have an extreme edge case like a file with 20k brackets, then the bracket query should return nothing. It will not be a deal breaker, because most of developers don't work with such huge files.

@hediet
Copy link
Member Author

hediet commented Aug 18, 2021

probably we should maximize this limit as big as possible, like 10k maybe.

I don't think that is a good idea - you should try to not query the brackets for the entire file on every keystroke. That is slow by design and absolutely does not work for files such as checker.ts with over 40k backet pairs.

Whatever the limit, 1k, 5k or 10k, I think Blockman cannot work with queried brackets of given range, it needs to know the entire file brackets, because the rendered blocks of current viewport window often can depend on the brackets which are located at the last line of the file or at the first line of the file, or both.

Hmm, do you really need all bracket pairs?
What about all those pairs whose range intersects a given range? This can be computed easily and if you only query the viewport, it should be always less than 1k.

Consider this:

{}
{
    {
        // start: a lot of bracket pairs here
        {}{}{}{}{}{}{}{}{}{}{}{}{}
        {}{}{}{}{}{}{}{}{}{}{}{}{}
        // end
    }

    // ...

    {
    // start of viewport
    }
    {
        {}
    }
    {
    // end of viewport
    }
}

Do you really need the brackets in "a lot of bracket pairs here" when providing the decorations for the current viewport?

@leodevbro
Copy link

It's pretty cool approach for optimization.

Well, yeah, if it is doable with not a huge pain, then
querying only the pairs whose range intersects a given viewport range
is very cool.

@leodevbro
Copy link

Got a question about the statement:

Extensions expect the tokens to be correct. However, with the current state of textmate and due to some performance optimizations, we cannot guarantee correctness.

It's from #128465

I see there are some feature updates about native bracket pair colorization. So, now, does the new version of VSCode guarantee correctness of bracket tokens? If no, then does querying only the pairs whose range intersects a given viewport range solve this problem? I mean, if the range will be limited with the viewport range, then the number of bracket tokens will always be small, so it means there will be no need for any more performance optimization, and it means there will be no more sacrifice of correctness.

@hediet,

Also, I would like to ask you, in what state is the feature idea of querying only the pairs whose range intersects a given viewport range? Is the VSCode team planning to implement it? Maybe in two months? Maybe in three months?

@hediet
Copy link
Member Author

hediet commented Nov 8, 2021

The problem is this:

const content = this.document.getText();
const bracketPairs = await vscode.languages.getBracketPairs(this.document.uri, this.getViewPortRange());
// use content and bracket pairs to create decorations

How can we prevent someone from doing this?
The issue here is that getBracketPairs is async. When the call returns, the text buffer could have already been modified and not agree with content anymore.

@leodevbro
Copy link

leodevbro commented Nov 8, 2021

getBracketPairs is async. When the call returns, the text buffer could have already been modified and not agree with content anymore.

In case of Blockman extension, I guess currently it works basically that way, when typing text on keyboard Blockman cannot analyze updated content quickly enough to keep up the changes in real time, so it deals with timers (debouncing) (It's like a refresh rate or frames per second in video games. If refresh rate is low, the game will have less CPU/GPU overload and will be more performant). On each text change event, Blockman starts re-analyzing the file after t seconds. If we set the t to be 0 seconds, the editor UI will most likely freeze during typing some text on the keyboard, so, that's why the default time is 1.2 seconds. The freezing is still noticeable during typing, but it's not a deal breaker for many coders because it feels rare and short.

Note: the timer is triggered only for the last change event and is not triggered on every event during the 1.2 seconds, so it will not get overloaded and bounced.

So, yeah, with Blockman, if text changes during the ongoing analyzing/rendering process, the blocks will be rendered for the old text, so it will look incorrect visually, but Blockman detects the change events and re-renders the blocks again with the new content. So the blocks finally are being rendered correctly. So, I guess the the blocks appear incorrect for about 1 second (average time) when working on a 5,000 line file, but if it will have access to the native tokens of brackets, I think this average time will go down to maybe 0.1 seconds, and so it will feel more comfy.

@heartacker
Copy link
Contributor

Are there any related investments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bracket-pair-colorization feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

3 participants