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

Faster DOM renderer #4605

Merged
merged 49 commits into from
Aug 1, 2023
Merged

Faster DOM renderer #4605

merged 49 commits into from
Aug 1, 2023

Conversation

jerch
Copy link
Member

@jerch jerch commented Jul 21, 2023

Fixes #4604.

TODO:

  • fix render glitches:
    • selection in merged spans does not show chars
    • cursor within merged span does not show char
    • extended attribs dont account yet for new span creation (see colored underline in underline test)
    • underlines on hovered links not working
    • check ligature joins (see notes below)
  • make metrics calc lazy / recalc on font changes
  • fix bad render runtime for metrics calc
  • fix bad runtime of merge logic
  • better codepoint selection for optimization
  • investigate whether to ignore tiny spacings - Solved by default spacing on all spans.
  • investigate, whether inline-block on spans is really needed (+20% render penalty) - Postponed for own PR.
  • make font weights configurable
  • avoid full re-render in handleSelectionChanged - Postponed.
  • test cases
  • create follow-up tickets/PRs from postponed issues:
  • change measuring to offsetWidth, see Fix for #2488. Use element offsetHeight rather than getBoundingRect #4366

Also fixes #4554.
Also fixes #4602.
Also fixes #4601.
Also fixes #4556.

@willmcgugan
Copy link

Looking forward to this one! 😀

@jerch
Copy link
Member Author

jerch commented Jul 21, 2023

Geez, the branch/commit history is very broken (prolly from my older attempts), needs serious rebasing...

@jerch
Copy link
Member Author

jerch commented Jul 22, 2023

@willmcgugan Indeed, this promises some serious perf boost for DOM renderer. Still it creates quite some glitches, idk yet if they all can be worked around.

@jerch
Copy link
Member Author

jerch commented Jul 22, 2023

@Tyriar Plz have a look at this profiler stats:

grafik

Is there a specific reason, why the selection service calls into renderRows, although I have nothing selected at all? Note that it also takes twice as long as from the render debouncer. This kinda looks like a big perf smell to me, but idk the inner works of the selection service.

@jerch
Copy link
Member Author

jerch commented Jul 22, 2023

@Tyriar The last commit can fix the superfluous paints from the selection service, saving ~1s runtime for my test. Would be good if you can have a look at it and check, whether thats a viable way to debounce the selection refreshs.

@jerch
Copy link
Member Author

jerch commented Jul 22, 2023

Eww - the link underline logic for mouse hovers needs a full rework with the merged spans, it simply cannot be done that way anymore 😢

Edit: Only way I found to solve this is by explicit re-rendering of the rows in the link range, not nice but gets the job done.

@jerch
Copy link
Member Author

jerch commented Jul 23, 2023

@Tyriar The following test is failing on windows only for no obvious reason:

it('should fire when writing to terminal', async () => {
await page.evaluate(`
window.calls = [];
window.search.onDidChangeResults(e => window.calls.push(e));
`);
await writeSync(page, 'abc bc c\\n\\r'.repeat(2));
assert.strictEqual(await page.evaluate(`window.search.findNext('abc', { decorations: { activeMatchColorOverviewRuler: '#ff0000' } })`), true);
assert.deepStrictEqual(await page.evaluate('window.calls'), [
{ resultCount: 2, resultIndex: 0 }
]);
await writeSync(page, 'abc bc c\\n\\r');
await timeout(300);
assert.deepStrictEqual(await page.evaluate('window.calls'), [
{ resultCount: 2, resultIndex: 0 },
{ resultCount: 3, resultIndex: 0 }
]);
});

Tested it manually under linux, where it works as expected. Since I have no working windows installation I cannot look into it. Any clue why this might be failing out of a sudden?

@jerch
Copy link
Member Author

jerch commented Jul 23, 2023

About ligature joins (manually tested):

  • View composition works as expected.
  • View on selection from the left side works as expected. From the right side the right-most chars still get blacked-out until the left-most of the ligature is selected too. Thats a quite weird behavior, not quite sure yet, whats causing this.
  • Moving the cursor into a ligature shows a similar misbehavior - on the left-most cell the whole ligature gets "cursored", for any other cell to the right the cursor vanishes.

(Sidenote: The current DOM and webgl renderer also have glitches for ligatures, maybe caused by something else. Also the issues dont look major/blocking to me.)

@jerch
Copy link
Member Author

jerch commented Jul 23, 2023

I'd say this implementation is ready for first alpha tests. It would be very helpful if anyone interested in testing this could run it with any sort of complicated cmdline tools, so we can spot left-over regressions.

@jerch
Copy link
Member Author

jerch commented Jul 24, 2023

Not sure yet what to do about the bold, italic and bold&italic font variants. Currently it is assumed, that a proper monospace font wont have different glyph widths for those, but this gonna break really bad if only the regular variant is available and the font renderer has to use a fallback font.

The issues with also calculating these variants are:

  • they are pretty rare in a terminal context, might not be worth to bother at all
  • multiplies calc runtime by 4 to [regular, bold, italic, bold&italic] - currently regular takes ~500ms for codepoint 32 to 1423
  • merge logic gets more cumbersome

Possible solutions:

  • blacklist them from the merge dropping back to single cell spans (re-introduces ugly glyph cuts in italic though)
  • calc them as well, but on a smaller codepoint range (e.g. ASCII only, should still address >>90% of terminal chars)

@mofux mofux self-requested a review August 1, 2023 11:33
Copy link
Contributor

@mofux mofux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it a good test and it works very well for me (Linux with Edge and Firefox). The performance is impressive - well done! It's really only the block characters where the WebGL renderer is still superior. There certainly are ways to also add those custom-drawn block elements to the DOM renderer, but that is a challenge for another day :)

@jerch
Copy link
Member Author

jerch commented Aug 1, 2023

@mofux Yeah the performance is quite impressive for the fact, that it is still DOM manipulations going on. Thx again for the letter-spacing trick, it is def. the better way to deal with it. As noted above, there are ~20% more perf bonus possible, if we manage to get the spans into inline, thus completely rely on the layouter to do its job. And it would solve all the over-/underline issues, and make Bidi partially work too.

About the block chars - yeah kinda all font renderer libs have issues with those in monospace, as far as I am aware almost all other TEs render those themselves to overcome the font lib shortcomings. I have not thought about a workaround for the DOM renderer yet, surely we can find here something.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a thorough review, looks close

src/browser/services/CharSizeService.ts Outdated Show resolved Hide resolved
src/browser/renderer/dom/WidthCache.ts Outdated Show resolved Hide resolved
src/browser/renderer/dom/WidthCache.ts Outdated Show resolved Hide resolved
src/browser/renderer/dom/WidthCache.ts Show resolved Hide resolved
src/browser/renderer/dom/WidthCache.ts Show resolved Hide resolved
src/browser/renderer/dom/WidthCache.ts Outdated Show resolved Hide resolved
src/browser/renderer/dom/DomRenderer.ts Show resolved Hide resolved
src/browser/renderer/dom/DomRenderer.ts Show resolved Hide resolved
Tyriar
Tyriar previously requested changes Aug 1, 2023
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a thorough review, looks close

@Tyriar
Copy link
Member

Tyriar commented Aug 1, 2023

Also there's a conflict

@Tyriar Tyriar added this to the 5.3.0 milestone Aug 1, 2023
@Tyriar Tyriar self-assigned this Aug 1, 2023
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like everything is checked off and comments resolved so let's merge and I'll pull it into VS Code so we can get some early testing. @jerch can you make the followup issues when you get a chance?

@jerch
Copy link
Member Author

jerch commented Aug 1, 2023

can you make the followup issues when you get a chance?

Yeah all done. 😺
So this PR closed 5 issues, but created 4 new ones, haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment