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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Character Measurements (iframe) #7945

Closed
wants to merge 61 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@as-cii
Member

as-cii commented Jul 17, 2015

This PR is intended to supersede #6083. Please, note that it鈥檚 still a 馃懛 work in progress 馃懛 and, as a result, there are still some minor hiccups and a few performance regressions, which I am going to expound later in this description.

Background

Have a look at #6055 to get a sense of why we want to do this. @nathansobo did a great job at explaining the drawbacks of the current approach and how we could possibly solve them, so I strongly recommend to read that issue in depth.

Solution: <iframe>

The idea with the approach proposed in this PR is to have an independent context where we can arbitrarily build a bunch of DOM nodes exclusively for measuring purposes. We employed an <iframe> for this, so that a reflow in the measurement context does not affect the main document and viceversa.

To accomplish this we have introduced LinesYardstick (naming is hard 馃槄), an abstraction whose purpose is to hide the complexity of dealing with the measurement context housekeeping. The API is simple:

  • buildDomNodesForScreenRows
  • leftPixelPositionForScreenPosition

We need to explicitly have two separate steps, as otherwise we鈥檇 end up continously changing the DOM and reading from it, thus causing extra reflows.

The Measurement Context

The context, as stated previously, is an <iframe> that replicates some of the functionalities featured on the fully fledged document, namely:

  • Has a bunch of line nodes
  • Includes a custom <style> element which contains font-size information
  • Includes <atom-styles> which, in turn, contains the syntax theme

You can think of it as a skinny replica of the larger document, where line characters occupy the exact same space and are, therefore, measurable. It鈥檚 extremely important for this context to be independent, as we will use it in TextEditorPresenter as well as in DisplayBuffer (in the future).

Performance

Side note: this section is a collection of personal thoughts and may not be accurate or necessarily correct. However, it will hopefully serve as a starting point for further optimizations and discussions.

We have encountered several performance regressions along the way, especially for performance sensitive scenarios such as scrolling. Keep in mind that for such intensive activities we need to respect the 16ms budget.

As of today, the steps we perform are the following:

  1. Right before updating (i.e. at the beginning of TextEditorPresenter#getState), we build the DOM nodes for a bunch of screen rows we know we are going to measure:
    • All the rows that will be rendered on screen after the update (to make sure decorations, cursors, etc. will be able to measure stuff).
    • The longest row (to calculate the maximum width of the open document).
    • The last cursor row (to calculate the hidden input position).
  2. As we update things (@updateDecorations, @updateCursors), we call @pixelPositionForScreenPosition which, in turn, will query LinesYardstick for positions. The first time this method is called, a Recalculate Style and a Layout will be triggered.

These are the two main operations and you can see how they perform on a quite heavy scrolling session (profiler disabled to avoid performance overhead):

screen shot 2015-07-17 at 11 59 41

And, if we zoom in one of those spikes:

screen shot 2015-07-17 at 12 01 21

The first half of the picture is the illustration of the aforementioned steps:

  1. Building HTML: 1.33ms
  2. Parsing HTML: 1.28ms
  3. Recalculate Style: 0.839ms
  4. Layout: 1.408ms

The whole frame took 20ms; measuring the first half via Chrome (Shift+Click) reported 6ms.

Now the question is: what could we improve? There are a couple of possibilities:

  1. Build the same HTML for the lines in the context as well as for the lines in the real document. This way we can avoid reparsing the lines twice, although cloning the nodes still takes some time. Not sure if this is really useful.

  2. Build skinnier lines for the measurement context. Ideally, we should avoid as much nesting as possible and, reducing the scopes complexity by building only those ones needed for styling, could improve that. It's also true that it would complicate things a bit (involves dealing with CSS rules, etc.). (eb8a1c0)

  3. Be smarter about updates. Instead of including every possible line, we build and layout only those lines needed for position-based updates. The drawback here is that, when e.g. we have decoration for each visible row, this optimization is useless. This means I want to make sure to speedup the current process instead of relying on not having certain rows. Anyway, this improvement has a positive impact on Layout, Recalculate Style and HTML manipulation. To get a sense of what this could look like, here's a relevant snippet:

    getMeasurableScreenRows: ->
      measurableRows = new Set
    
      @addOverlaysRowsToSet(measurableRows)
      @addCursorsRowsToSet(measurableRows)
      @addDecorationsRowsToSet(measurableRows)
    
      if lastCursorRange = @model.getLastCursor()?.getScreenRange()
        measurableRows.add(lastCursorRange.start.row)
        measurableRows.add(lastCursorRange.end.row)
    
      if longestScreenRow = @model.getLongestScreenRow()
        measurableRows.add(longestScreenRow)
    
      measurableRows
  4. Cut non-syntax CSS from styles. Removing <atom-styles> does not yield huge speedups, so I am not sure it's the first thing we want to improve. Still an improvement I鈥檇 like to make, though. (2257688)

  5. Reduce layout times by trying to modify the measurement context without affecting the whole document. If we manage to always change the DOM in a way that affects only smaller layout boundaries, we may be able to halve reflow times. (28405e0)

I believe a combination of 2), 3), 4) and 5) could be interesting. HTML creation (and parsing) and Layout are the expensive operations right now and I am thinking to ways in which we could speed them up. I am still kinda making some research but I feel we could reach decent performance if we put some more effort on this. I'll investigate other potential speedups in the meantime. @nathansobo: what do you think?

Conclusion

If you have read everything so far, I extremely appreciate your attention. I feel like there are many ways to solve this problem, none of which is guaranteed to be the right one, thus I wanted to include as much of my knowledge as possible so that you and the future me can try things out based on it. 馃檹

Any feedback would be much appreciated! 馃檱 Thanks!

as-cii added some commits Jul 7, 2015

Use the same HTML on the measuring context
Although a bit slower, this will allow us to exploit caching when
working with real lines, so that we can pay for lines DOM nodes
construction just once.
馃挌 Wait for `linesYardstick` to be fully initialized
鈥ecause `iframe` gets loaded asynchronously and we don鈥檛 want to
trigger updates before it is initialized.
馃帹 Start crafting `pixelPositionForScreenPosition`
We are still faking the stylesheet, but hopefully it will be super easy
to make the switch with a super minimal, yet realistic, one.

* Ensure `TextEditorComponent::pixelPositionForScreenPosition` behaves
exactly as `TextEditorPresenter`鈥檚 one
馃挌 Adapt specs to the new tokens HTML
We needed to take into account the new `<span class='token'>鈥</span>`,
although the behavior hasn't actually changed.
Split `updateContentDimensions`
鈥o that we can first know which lines are visible and then use the
measuring context to compute the maximum width.
Guard against unconstructed line nodes
This is a stopgap measure until we understand what鈥檚 causing DOM nodes
not to be built.

@benogle benogle referenced this pull request Jul 22, 2015

Closed

The Quality Initiative #7995

66 of 121 tasks complete
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jul 22, 2015

Contributor

Just got a stack trace using this...

TypeError: Cannot read property 'textContent' of undefined
  at LinesYardstick.module.exports.LinesYardstick.leftPixelPositionForCharInTextNode (/Applications/Atom.app/Contents/Resources/app.asar/src/lines-yardstick.js:257:52)
  at LinesYardstick.module.exports.LinesYardstick.leftPixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/lines-yardstick.js:226:19)
  at TextEditorComponent.module.exports.TextEditorComponent.pixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:1076:34)
  at TextEditorPresenter.module.exports.TextEditorPresenter.pixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:1367:33)
  at TextEditorPresenter.module.exports.TextEditorPresenter.pixelRectForScreenRange (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:1391:22)
  at TextEditorPresenter.module.exports.TextEditorPresenter.updateHiddenInputState (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:420:20)
  at TextEditorPresenter.module.exports.TextEditorPresenter.getState (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:152:14)
  at TextEditorComponent.module.exports.TextEditorComponent.updateSync (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:197:38)
  at /Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:313:28
  at ViewRegistry.module.exports.ViewRegistry.performDocumentUpdate (/Applications/Atom.app/Contents/Resources/app.asar/src/view-registry.js:168:9)
  at /Applications/Atom.app/Contents/Resources/app.asar/src/view-registry.js:3:61

Trying to position my cursor at the end of line 2713 of this file.

Contributor

nathansobo commented Jul 22, 2015

Just got a stack trace using this...

TypeError: Cannot read property 'textContent' of undefined
  at LinesYardstick.module.exports.LinesYardstick.leftPixelPositionForCharInTextNode (/Applications/Atom.app/Contents/Resources/app.asar/src/lines-yardstick.js:257:52)
  at LinesYardstick.module.exports.LinesYardstick.leftPixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/lines-yardstick.js:226:19)
  at TextEditorComponent.module.exports.TextEditorComponent.pixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:1076:34)
  at TextEditorPresenter.module.exports.TextEditorPresenter.pixelPositionForScreenPosition (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:1367:33)
  at TextEditorPresenter.module.exports.TextEditorPresenter.pixelRectForScreenRange (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:1391:22)
  at TextEditorPresenter.module.exports.TextEditorPresenter.updateHiddenInputState (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:420:20)
  at TextEditorPresenter.module.exports.TextEditorPresenter.getState (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-presenter.js:152:14)
  at TextEditorComponent.module.exports.TextEditorComponent.updateSync (/Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:197:38)
  at /Applications/Atom.app/Contents/Resources/app.asar/src/text-editor-component.js:313:28
  at ViewRegistry.module.exports.ViewRegistry.performDocumentUpdate (/Applications/Atom.app/Contents/Resources/app.asar/src/view-registry.js:168:9)
  at /Applications/Atom.app/Contents/Resources/app.asar/src/view-registry.js:3:61

Trying to position my cursor at the end of line 2713 of this file.

as-cii added some commits Jul 23, 2015

馃悰 Guard against empty trailing whitespace
An empty trailing whitespace was being generated and this caused empty
trailing tokens to be appended to the HTML, thereby screwing up the
binary search.

/cc: @nathansobo
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Jul 23, 2015

Contributor

馃憤

Contributor

nathansobo commented on 377771c Jul 23, 2015

馃憤

@as-cii as-cii referenced this pull request Sep 16, 2015

Merged

DOM-based measurements #8811

11 of 11 tasks complete

@the-solipsist the-solipsist referenced this pull request Feb 19, 2016

Open

Headers #20

@as-cii as-cii deleted the as-iframe-measurements branch Feb 22, 2016

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