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

Only update overlay instead of text editor when resize occurs #15954

Merged
merged 3 commits into from Oct 23, 2017

Conversation

Projects
None yet
4 participants
@leroix
Contributor

leroix commented Oct 20, 2017

We took care of an unnecessary loop that was occurring when an overlay was present in #15894. However, resizes of the overlay were still taking longer than needed because they were invoking .updateSync on the text editor component. In this PR, we take the approach of only updating the dimensions of the overlay element and invoking a render on just the overlay component when a resize occurs. The amount of time spent in the overlay observer callback is greatly reduced which means less time spent scripting on keystrokes. This is important because any scripting that occurs as a result of a new keystroke needs to finish well ahead of the next keystroke to avoid holding up its handling.

overlay_resizing

In the above image, the time spent in the resize observer callback is reduced from 2.36ms to 0.32ms, and the time to handle this frame goes from 11.75ms down to 9.10ms.

/cc @nathansobo

Show outdated Hide outdated spec/text-editor-component-spec.js Outdated
updateOverlaysToRender () {
const overlayCount = this.decorationsToRender.overlays.length
if (overlayCount === 0) return null
const windowInnerHeight = this.getWindowInnerHeight()
const windowInnerWidth = this.getWindowInnerWidth()
const contentClientRect = this.refs.content.getBoundingClientRect()
for (let i = 0; i < overlayCount; i++) {

This comment has been minimized.

@Ingramz

Ingramz Oct 21, 2017

Contributor

Possible to replace with this.decorationsToRender.overlays.forEach(this.updateOverlayToRender) ? Edit: probably not, might be slower if we are after performance.

@Ingramz

Ingramz Oct 21, 2017

Contributor

Possible to replace with this.decorationsToRender.overlays.forEach(this.updateOverlayToRender) ? Edit: probably not, might be slower if we are after performance.

This comment has been minimized.

@leroix

leroix Oct 21, 2017

Contributor

That's a good point and switching to .forEach may have a minimal impact on performance, but I haven't measured it.

@leroix

leroix Oct 21, 2017

Contributor

That's a good point and switching to .forEach may have a minimal impact on performance, but I haven't measured it.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Oct 23, 2017

Contributor

⚡️ Awesome speedup!

Contributor

maxbrunsfeld commented Oct 23, 2017

⚡️ Awesome speedup!

@@ -804,7 +804,15 @@ class TextEditorComponent {
key: overlayProps.element,
overlayComponents: this.overlayComponents,
measuredDimensions: this.overlayDimensionsByElement.get(overlayProps.element),
didResize: () => { this.updateSync() }
didResize: (overlayComponent) => {

This comment has been minimized.

@nathansobo

nathansobo Oct 23, 2017

Contributor

Good idea passing in the component reference in this callback. This is such a twisty data relationship but I think this is a reasonable way to navigate it.

@nathansobo

nathansobo Oct 23, 2017

Contributor

Good idea passing in the component reference in this callback. This is such a twisty data relationship but I think this is a reasonable way to navigate it.

@nathansobo

Looking forward to this improvement.

@leroix leroix merged commit c266276 into master Oct 23, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix deleted the faster-overlay-resizing branch Oct 23, 2017

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