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

Implement text editor DOM updates manually instead of via React #5624

Merged
merged 40 commits into from Feb 20, 2015

Conversation

@nathansobo
Copy link
Contributor

nathansobo commented Feb 18, 2015

React is an amazing abstraction, but very few abstractions come without at least some overhead. In the case of Atom's text editor, it's worth the effort to avoid this overhead by hand coding all DOM updates. The update code is repetitive, but it's also pretty straightforward. In exchange, we get much simpler profile traces, drop a big dependency, and reduce the overall number of moving parts in Atom. Below are some flame graphs for entering a character at the end of a syntax-highlighted line. The comparison is somewhat unfair because the React case could have been a bit more optimized, but I think the pictures also show how much simpler the manual approach is in our case.

screenshot_2015-02-18_15_03_39

screenshot_2015-02-18_15_01_42

React is a great tool for many cases, for our particular needs in this particular case I decided it would be easier to just do things for ourselves.

Remaining Tasks:

  • Don't use React for EditorComponent
  • Fix autocomplete spec.
  • Unify DOM updates / polling across editors.
@benogle
Copy link
Contributor

benogle commented Feb 18, 2015

Legit

@nathansobo
Copy link
Contributor Author

nathansobo commented Feb 19, 2015

I'm going to keep the basic structure of the code with the TextEditorComponent separate from the TextEditorElement the same for now. It might be nice to clean it up but it's not really a huge deal and I think time could be spent better in other ways. Someday I would love to get to a more minimal DOM structure as well, but it doesn't seem worth the risk of breaking anything to lose a couple extra nodes.

@nathansobo nathansobo force-pushed the ns-manual-dom-updates branch from cd66ea9 to cae5bf5 Feb 19, 2015
nathansobo added 27 commits Feb 10, 2015
Also, remove ability to disable hardware acceleration since there’s
no longer a need for it and it complicated this conversion.
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Signed-off-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
@lee-dohm
Copy link
Member

lee-dohm commented Feb 20, 2015

@nathansobo I've been working with it for the past hour. I haven't noticed any issues. But it does seem a bit more responsive 😀

@thomasjo
Copy link
Member

thomasjo commented Feb 20, 2015

Been using this all day and haven't noticed any regressions. The constant console logging is very annoying though, hehe 👼

@thomasjo
Copy link
Member

thomasjo commented Feb 20, 2015

I've not really noticed any major performance issues in the past few months, so hard to tell whether the speedup is noticeable or not on my machine, but things do seem a bit more responsive. Could be confirmation bias though.

@postcasio
Copy link
Contributor

postcasio commented Feb 20, 2015

Same here. No issues so far, playing well with my other packages. Scrolling is silky smooth!

@as-cii
Copy link
Contributor

as-cii commented Feb 20, 2015

Same here, it has been working fine so far. Great job, @nathansobo! 👍

The blinking cursor was ensuring that we never polled in certain cases.
We need to allow the interval to continue polling at a normal pace, but
just avoid doing any work that could delay the next animation frame.
@nathansobo nathansobo force-pushed the ns-manual-dom-updates branch from 075f7eb to 32d393d Feb 20, 2015
@nathansobo
Copy link
Contributor Author

nathansobo commented Feb 20, 2015

Thanks to all of you for your time! @thomasjo thanks for pointing out the console logging. I accidentally left it in on the last commit and just force-pushed an amendment.

nathansobo added a commit that referenced this pull request Feb 20, 2015
WIP: Implement text editor DOM updates manually instead of via React
@nathansobo nathansobo merged commit f4116c7 into master Feb 20, 2015
0 of 4 checks passed
0 of 4 checks passed
atom Build #1910483 started
Details
atom-linux Build #1910484 started
Details
atom-rpm Build #1910485 started
Details
atom-win Build #1910486 started
Details
@nathansobo nathansobo deleted the ns-manual-dom-updates branch Feb 20, 2015
@kevinsawicki
Copy link
Member

kevinsawicki commented Feb 20, 2015

📈 💚 ♻️

@lee-dohm
Copy link
Member

lee-dohm commented Feb 20, 2015

👏 Awesome work!

@nathansobo
Copy link
Contributor Author

nathansobo commented Feb 20, 2015

I force pushed back the merge of this PR so we can get a release out this morning without me having to worry about this code over the weekend. Will merge it again once the release is out.

@performDocumentPollAfterUpdate = true
else
@performDocumentPollAfterUpdate = false
poller() for poller in @documentPollers

This comment has been minimized.

Copy link
@kevinsawicki

kevinsawicki Feb 20, 2015

Member

Might want to return after this line to prevent array return.

This comment has been minimized.

Copy link
@nathansobo

nathansobo Feb 20, 2015

Author Contributor

👍 Very good catch.

@nathansobo nathansobo restored the ns-manual-dom-updates branch Feb 20, 2015
@nathansobo
Copy link
Contributor Author

nathansobo commented Feb 20, 2015

Okay, merged in 9648093. Sorry for the false 🚨.

@nathansobo nathansobo changed the title WIP: Implement text editor DOM updates manually instead of via React Implement text editor DOM updates manually instead of via React Feb 25, 2015
@hemanth
Copy link

hemanth commented Mar 1, 2015

Interesting stats, but was curious about why was react opted before...

@nathansobo
Copy link
Contributor Author

nathansobo commented Mar 1, 2015

@hemanth There was also a huge improvement over the first version of the editor when we added React. It was much like scaffolding.

@hemanth
Copy link

hemanth commented Mar 1, 2015

👍

@mcharytoniuk
Copy link

mcharytoniuk commented Mar 1, 2015

Why didn't you use React's PureRenderMixin (http://facebook.github.io/react/docs/pure-render-mixin.html), 'shouldComponentUpdate' custom implementation, batched updates and other optimisations that would produce better results than manual updates and abandoned the entire solution rapidly instead?

@kl3ryk
Copy link

kl3ryk commented Mar 1, 2015

@benogle
Copy link
Contributor

benogle commented Mar 2, 2015

@mcharytoniuk The old implementation was doing the same thing as described in the doc page:

Under the hood, the mixin implements shouldComponentUpdate, in which it compares the current props and state with the next ones and returns false if the equalities pass.

See

shouldComponentUpdate: (newProps) ->
return true unless isEqualForProperties(newProps, @props,
'renderedRowRange', 'lineDecorations', 'highlightDecorations', 'lineHeightInPixels', 'defaultCharWidth',
'overlayDecorations', 'scrollTop', 'scrollLeft', 'showIndentGuide', 'scrollingVertically', 'visible',
'scrollViewHeight', 'mouseWheelScreenRow', 'scopedCharacterWidthsChangeCount', 'lineWidth', 'useHardwareAcceleration',
'placeholderText', 'performedInitialMeasurement', 'backgroundColor', 'cursorPixelRects'
)
{renderedRowRange, pendingChanges} = newProps
return false unless renderedRowRange?
[renderedStartRow, renderedEndRow] = renderedRowRange
for change in pendingChanges
if change.screenDelta is 0
return true unless change.end < renderedStartRow or renderedEndRow <= change.start
else
return true unless renderedEndRow <= change.start
false

@mcharytoniuk
Copy link

mcharytoniuk commented Mar 2, 2015

I know that maybe it's too late for such ideas but I'd try moving away at least cursor component out of lines component and position it absolutely on top of it to prevent lines component updates caused by cursor movement. Also, Flipboard published react-canvas recently. Did you ever consider using canvas for animations / some editor elements as they did? Many people criticised them for negative SEO impact but inside text editor every crazy trick is acceptable imo. :)

@nathansobo
Copy link
Contributor Author

nathansobo commented Mar 2, 2015

A big selling-point of Atom is the ability to style content with CSS. Used properly, the DOM doesn't introduce prohibitive overhead for our use case.

React being removed is purely an implementation detail in this case and shouldn't affect package authors in any way. Appreciate your concern and suggestions here, but on balance it's easier to achieve our goals without it in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.