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

Move towards more efficient DOM manipulation #900

Merged
merged 3 commits into from Oct 5, 2017

Conversation

Projects
None yet
3 participants
@leroix
Contributor

leroix commented Oct 5, 2017

Description of the Change

This PR moves away from setting .innerHTML in .renderItem where possible. Though, when we receive HTML via the suggestion provider API, we still have to use it. Direct DOM manipulation tends to be faster when called in a loop:
screen shot 2017-10-03 at 11 05 19 pm

screen shot 2017-10-03 at 11 05 46 pm

Here's an illustration of the improvement for the same autocomplete scenario before and after the changes in this PR:
screen shot 2017-10-04 at 6 49 01 pm
screen shot 2017-10-04 at 6 48 04 pm

Alternate Designs

n/a

Benefits

This is a step towards faster rendering of autocomplete suggestions making the typing experience feel more responsive.

Possible Drawbacks

Imperative dom manipulation can potentially be more complex than just setting .innerHTML in some cases.

Applicable Issues

n/a

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Oct 5, 2017

Contributor

⚡️ Great to see that big of a speed improvement!

Code looks good to me, though I've never worked on this code before.

Contributor

maxbrunsfeld commented Oct 5, 2017

⚡️ Great to see that big of a speed improvement!

Code looks good to me, though I've never worked on this code before.

@nathansobo nathansobo changed the title from move towards more efficient DOM manipulation to Move towards more efficient DOM manipulation Oct 5, 2017

@nathansobo

I like your approach to updating the tests. If the test coverage was good previously, then this should be fine. There was quite a bit of logic for constructing the elements and I'd be lying if I looked at each line super closely, but from a few minutes of inspection it all seems reasonable. I say we move forward with this. Nice work!

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 5, 2017

Contributor

It's probably unrelated to the content of this PR, but What's up with the subsequence provider failing against beta on AppVeyor? Did we change the behavior?

Contributor

nathansobo commented Oct 5, 2017

It's probably unrelated to the content of this PR, but What's up with the subsequence provider failing against beta on AppVeyor? Did we change the behavior?

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 5, 2017

Contributor

@nathansobo Yea, that's what I'm looking into today. I don't think we've changed the behavior that I know of. I'm a bit befuddled.

Btw, I don't really like getDisplay either. I was just having trouble thinking of something better.

Contributor

leroix commented Oct 5, 2017

@nathansobo Yea, that's what I'm looking into today. I don't think we've changed the behavior that I know of. I'm a bit befuddled.

Btw, I don't really like getDisplay either. I was just having trouble thinking of something better.

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 5, 2017

Contributor

I just built 1.22.0-beta0 locally, ran the specs, and didn't get the failure that was present in Appveyor. 😕

Contributor

leroix commented Oct 5, 2017

I just built 1.22.0-beta0 locally, ran the specs, and didn't get the failure that was present in Appveyor. 😕

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 5, 2017

Contributor

We figured out that appveyor is failing because of an ambiguity in the sorting logic in https://github.com/atom/superstring/blob/master/src/core/text-buffer.cc#L525

If the scores are the same, the order is left up to the order the matches showed up in unordered_map which is platform-dependent.

The fix for this is unrelated to this PR, so I think this PR is ready to go.

Contributor

leroix commented Oct 5, 2017

We figured out that appveyor is failing because of an ambiguity in the sorting logic in https://github.com/atom/superstring/blob/master/src/core/text-buffer.cc#L525

If the scores are the same, the order is left up to the order the matches showed up in unordered_map which is platform-dependent.

The fix for this is unrelated to this PR, so I think this PR is ready to go.

@leroix leroix merged commit 8a3a0be into atom:master Oct 5, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix deleted the leroix:faster-rendering branch Oct 5, 2017

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