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

Fix overlay resizing infinite loop #15894

Merged
merged 1 commit into from Oct 16, 2017

Conversation

Projects
None yet
3 participants
@leroix
Contributor

leroix commented Oct 15, 2017

When an overlay is present, we are comparing the dimensions of the overlay's child with the dimensions of atom-overlay. If they are different, we disconnect the resize observer, call the .didResize callback, and observe atom-overlay once more on the next tick. In autocomplete-plus (possibly others), the child of the overlay never matches the dimensions of the overlay, and the resize observer callback gets triggered on every tick as long as the suggestion list is present.

This bug is particularly noticeable when a large file like text-editor-component.js is opened and completely folded. Here are timelines before and after this fix when a suggestion list for the same prefix at the same editor position is opened:

before
screen shot 2017-10-14 at 6 59 32 pm

after
screen shot 2017-10-14 at 6 55 19 pm

Here's a zoomed in shot of the timeline before the fix:
screen shot 2017-10-14 at 7 00 09 pm

@nathansobo

Excellent description and the fix looks good! We should hotfix this to beta and stable.

@leroix leroix merged commit 83cb0b9 into atom:master Oct 16, 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 leroix:jr-overlay-resize-bug branch Oct 16, 2017

leroix added a commit that referenced this pull request Oct 16, 2017

Merge pull request #15894 from leroix/jr-overlay-resize-bug
Fix overlay resizing infinite loop

leroix added a commit that referenced this pull request Oct 16, 2017

Merge pull request #15894 from leroix/jr-overlay-resize-bug
Fix overlay resizing infinite loop
@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 2, 2017

I think this commit may be causing my package Jumpy to slowdown (went from almost instant in most cases to unusably slow):

Here's an image of what happens after I toggle the jump mode to display the labels:
image

Should I be using a DisplayMarkerLayer and LayerDecoration? Do I need to jump on that bandwagon? :-)

DavidLGoldberg commented Nov 2, 2017

I think this commit may be causing my package Jumpy to slowdown (went from almost instant in most cases to unusably slow):

Here's an image of what happens after I toggle the jump mode to display the labels:
image

Should I be using a DisplayMarkerLayer and LayerDecoration? Do I need to jump on that bandwagon? :-)

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 2, 2017

Contributor

@DavidLGoldberg Maybe something about the change in logic is causing us to re-measure overlay decorations more than needed. @leroix you should probably install his package and see what's happening. Thanks for the report @DavidLGoldberg.

Contributor

nathansobo commented Nov 2, 2017

@DavidLGoldberg Maybe something about the change in logic is causing us to re-measure overlay decorations more than needed. @leroix you should probably install his package and see what's happening. Thanks for the report @DavidLGoldberg.

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 2, 2017

No problem. Thanks for taking this seriously. I know this isn't exactly the easiest part of Atom ;)

I just updated my xcode and will be trying to do a build of atom with the diff to see if that's the trouble spot. Although, I've actually never had to build atom core before so wish me luck :)

DavidLGoldberg commented Nov 2, 2017

No problem. Thanks for taking this seriously. I know this isn't exactly the easiest part of Atom ;)

I just updated my xcode and will be trying to do a build of atom with the diff to see if that's the trouble spot. Although, I've actually never had to build atom core before so wish me luck :)

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Nov 2, 2017

Contributor

@DavidLGoldberg @nathansobo apparently, contentRect in the ResizeObserver api is different from the bounding box returned by getBoundingClientRect().

https://wicg.github.io/ResizeObserver/
https://medium.com/dev-channel/after-mutationobserver-performanceobserver-and-intersectionobserver-we-have-another-observer-for-2c541bcb531b

So, we're comparing apples and oranges here except most of the time the apples look like oranges.

I'm not sure of the fix yet. Could we just call getBoundingClientRect() instead of using contentRect? Would this force a recalc?

Contributor

leroix commented Nov 2, 2017

@DavidLGoldberg @nathansobo apparently, contentRect in the ResizeObserver api is different from the bounding box returned by getBoundingClientRect().

https://wicg.github.io/ResizeObserver/
https://medium.com/dev-channel/after-mutationobserver-performanceobserver-and-intersectionobserver-we-have-another-observer-for-2c541bcb531b

So, we're comparing apples and oranges here except most of the time the apples look like oranges.

I'm not sure of the fix yet. Could we just call getBoundingClientRect() instead of using contentRect? Would this force a recalc?

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 2, 2017

Ah just seeing the above now (I'll try to process that stuff tomorrow :) )

If anyone's interested I ran a series of tests between atom, atom-beta, and dev build...with both old package and a new one I've been working on (rewrite). The results are kind of interesting, almost seem to have to do with configuration of files on the system?

Why would both of my branches on the locally built dev be fast without long running warnings. I followed the directions for the build ...did a direct clone -> stayed on master while I built.

Did someone else happen to have already fix said issue inadvertently?

The below are my results: NOTE: I quit all between each.

atom . (regular stable install)
long running

atom-beta . (regular beta install)
long running

atom -d . (regular stable install) + new branch
fast!!!

atom-beta -d . (regular beta install) + new branch
fast!!!

/Applications/Atom.app/Contents/MacOS/Atom .
fast!!! (clearly older version of jumpy is definitely running, but still faster + none of the long running msgs)

/Applications/Atom.app/Contents/MacOS/Atom -d .
fast!!!

The above test matrix was actually repeatable.

The one interesting caveat is that prior to reinstall from downloadable (which I did first after my first dev install with locally built atom) even my -d with the new Jumpy branch was slow (or at least I'm 98% sure I tested this 2 or 3 times with the -d on the new branch and it was still slow). Whether it was slow or not at that point of my file system's configuration is moot in that, prior to release of course Jumpy wasn't so slow.

Perhaps running the dev env once changed some config that let the -d's work faster across the board?

DavidLGoldberg commented Nov 2, 2017

Ah just seeing the above now (I'll try to process that stuff tomorrow :) )

If anyone's interested I ran a series of tests between atom, atom-beta, and dev build...with both old package and a new one I've been working on (rewrite). The results are kind of interesting, almost seem to have to do with configuration of files on the system?

Why would both of my branches on the locally built dev be fast without long running warnings. I followed the directions for the build ...did a direct clone -> stayed on master while I built.

Did someone else happen to have already fix said issue inadvertently?

The below are my results: NOTE: I quit all between each.

atom . (regular stable install)
long running

atom-beta . (regular beta install)
long running

atom -d . (regular stable install) + new branch
fast!!!

atom-beta -d . (regular beta install) + new branch
fast!!!

/Applications/Atom.app/Contents/MacOS/Atom .
fast!!! (clearly older version of jumpy is definitely running, but still faster + none of the long running msgs)

/Applications/Atom.app/Contents/MacOS/Atom -d .
fast!!!

The above test matrix was actually repeatable.

The one interesting caveat is that prior to reinstall from downloadable (which I did first after my first dev install with locally built atom) even my -d with the new Jumpy branch was slow (or at least I'm 98% sure I tested this 2 or 3 times with the -d on the new branch and it was still slow). Whether it was slow or not at that point of my file system's configuration is moot in that, prior to release of course Jumpy wasn't so slow.

Perhaps running the dev env once changed some config that let the -d's work faster across the board?

@DavidLGoldberg

This comment has been minimized.

Show comment
Hide comment
@DavidLGoldberg

DavidLGoldberg Nov 2, 2017

Also, I wonder if anyone's done ResizeObserver with Mobx heh...

DavidLGoldberg commented Nov 2, 2017

Also, I wonder if anyone's done ResizeObserver with Mobx heh...

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