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

Causes severe performance problems with (Google) Sheets + Docs + Slides web apps #127

Closed
MarcusJTWhich opened this issue Dec 15, 2017 · 14 comments
Assignees
Labels
Milestone

Comments

@MarcusJTWhich
Copy link

For the last few weeks it's been almost impossible to use the Google Drive web apps Sheets, Docs and Slides (but not Drive itself, oddly) because there's been a 2-second delay or so every time I try do something (anything, even typing a single word results in a couple of characters appearing at a time with seconds in between) which has been excruciatingly frustrating and severely impacted my ability to do my job... I eventually discovered that the root cause was the Landmarks extension for Chrome (hence this bug report) by using the Performance tab of Chrome Developer Tools to trace what was going on when I tried to do anything in those web apps. I doubt I'm the only person suffering this excruciating slowdown for no apparent reason as a result of your extension so please investigate and resolve this issue. Meanwhile removing or disabling the extension returns performance back to normal.

@matatk matatk added the bug label Dec 18, 2017
@matatk
Copy link
Owner

matatk commented Dec 18, 2017

Hi @MarcusJTWhich. Sorry to hear that Landmarks has been causing performance issues for you, and thank you for letting me know so that I can look into it. I recently changed the extension quite significantly to reflect changes to landmarks that happen whenever the web page changes (previously it only scanned for landmarks on page load, which meant landmarks that encompass navigation panels and the like were often missed). I've not had time to look into the profiling on Chrome yet, but I think it may be to do with that change.

I have a question for you: does this also happen in Firefox when you have Landmarks installed there?

Assuming that it is down to this recent change, there are a few things I can think of to address/fix the problem, from shorter to longer term...

  • Introducing the ability to turn off the continual monitoring for landmark changes on a per-site basis, and adding some sites to this list by default, at least until I can figure out how to improve the performance.
    • If at all possible it'd be far better not to need this, so I'd aim to keep the UI for it minimal, probably just within the extension's options page.
  • In the medium term: improving the monitoring so it's more efficient.

I develop Landmarks in my own time, but I'll do what I can to investigate and sort this out. Without feedback, I may not be aware of problems (though I test it as extensively as possible), so thanks for your bug report. I'll keep you updated via this thread with progress.

@matatk
Copy link
Owner

matatk commented Dec 19, 2017

I thought of another approach to rate-limiting the scanning for landmark changes that might work automatically on any site: Landmarks could check to see how many times it's being told by the browser about changes on the page over a period of time and, if it's too high, it could ask the browser to stop telling it about those changes.

There are several questions around making sure this would actually work and how to ensure that it behaves consistently and the UI is kept as minimal as possible, but this is another idea that may help.

(I've not actually had time to check out the profiling yet, but wanted to record this thought whilst I had it.)

@MarcusJTWhich
Copy link
Author

MarcusJTWhich commented Dec 19, 2017

Aha! Yes, there are two techniques called throttling and debouncing that are crucial to apply to event listeners to avoid performance problems (it's most often encountered with scroll event listeners and results in animation jank) so if you're not doing either already that certainly explains the issues I'm encountering as Google Docs/Slides/Sheets must be making lots of DOM changes behind the scenes... read up at https://css-tricks.com/debouncing-throttling-explained-examples/ to decide which is most appropriate, and I recommend using https://lodash.com/ rather than underscore.js

@matatk
Copy link
Owner

matatk commented Dec 28, 2017

Thanks for that info, the article was a very interesting read; the demos were great too. Actually Landmarks uses a mutation observer as opposed to event listeners, but they are similar.

The mutation observer tells Landmarks of any changes that are made to the DOM. Landmarks (the extension) then decides if a full-page scan for landmarks is necessary, based on those changes. It takes about 1ms to decide, and can take 50ms or even longer on some pages to do the scan. If the mutation observer handler were debounced, Landmarks would not have access to the earlier records of what changed in the DOM, and would just have to do a full-page scan regardless—though that's not a huge performance issue, as the debouncing would have helped loads anyway.

There is another fairly big potential issue, though: even with debouncing, the performance overhead in the browser itself of using mutation observers on the <body> may simply be too great for some pages. I have not yet been able to determine if that's the case—perhaps you could help me with this, when I have a test version ready.

If using mutation observers on very dynamic pages is the core problem here, then I'll need to come up with a decent way of backing off from using them on pages that change too much, and some user-friendly way of explaining that.

Next steps seem to be:

  1. Try this all out for myself.
  2. See what happens when the mutation observer handler is debounced.
  3. If performance is still an issue: come up with a reasonable way to back off from using the mutation observer completely (would be a shame if this has to happen, but I can imagine for some pages, like games, maybe, that are just not what Landmarks expects, it may be necessary).

One further issue with respect to all the testing: Mozilla allows me to upload test versions of the extension so we could test it before releasing to everyone. I don't think I can do this in Chrome, so I will need to test in both, and maybe only try a test fix in Firefox.

@MarcusJTWhich
Copy link
Author

even with debouncing, the performance overhead in the browser itself of using mutation observers on the may simply be too great for some pages.

That's probably why Google Docs/Sheets/Slides have been having issues as they are highly complex web apps creating and updating huge DOMs with every interaction/keypress/etc so the overhead is too much.

Part of the solution could be making the Landmarks extension detect when the performance overhead is too much (e.g. >16ms so as not to break 60fps rendering), pause page scanning by the extension, and prompt the user to exclude the site (i.e. add it to an internal blacklist which can be edited by the user). This would prevent any users ever experiencing what I experienced.

As for testing extensions in Chrome, many extensions publish a "beta" version as either a public or an unlisted extension (see https://stackoverflow.com/questions/23033335/private-beta-testing-a-chrome-extension)

@matatk matatk self-assigned this Jan 6, 2018
@matatk
Copy link
Owner

matatk commented Jan 7, 2018

I've worked on this over the past couple of days. Fortunately it seems that the main bottleneck is in the scanning in the Landmarks extension, rather than the Mutation Observer stuff in the browser, so I have been able to implement a solution fairly simply.

(Interestingly, Firefox didn't have a really noticeable slowdown, but having checked out what is going on here, it was clearly the right thing to make Landmarks less resource-hungry in general.)

Here's what I have implemented:

  1. As it did before, Landmarks starts listening for all changes (mutations) to the page, and remains doing so.
  2. When a change occurs, it checks to see if it should scan for landmarks.
  3. There is then a "pause" before it will react immediately to any changes that follow. Because pages like Google Docs have a lot going on at once, more changes come through during this "pause" time.
  4. If one or more changes come through during the pause time, the pause time is increased and a scan for landmarks is scheduled for the end of the now-increased pause (because we can't know by then if we should have scanned or not, so need to run a full one). This stops things going slow because of an overwhelming number of changes coming through at once.
  5. We get to the end of the pause period and the scheduled scan is run.
  6. After this point, the pause period starts to decrease over time, so that if things quieten down in future, we can still react to changes to the page quickly. If a single change to the page happens, it will be handled immediately. If any others follow quickly, the pause will be increased again, as per step 4.

The pause increases exponentially (so it errs on the side of improving performance at the risk of not reacting to landmark changes quickly) and the decrease is linear (for the same reason). It's a bit like debouncing plus TCP's flow control (sort-of). Also reminds me a bit of Quake 3, where if one has health or armour above 100, it gradually falls back down to 100, hehe.

My hope is that adopting this technique will mean that it can adapt to pages with different interaction levels automatically. The nature of how quickly it backs off when changes occur, and how quickly it returns to being highly reactive, may need tweaking though.

Further things to note:

  • I'm thinking that it might be a good idea to check how out-of-date the current landmarks on the page are when the user uses the browser button, or the keyboard shortcuts, and maybe do a scan then if need be. Haven't implemented that yet. (This has since been implemented.)
  • I have included a setting in Landmarks' options page that allows you to turn on debugging messages in the console, so you can observe what the pause is and how it's being changed. I hope to be able to remove this in a future release. There's a couple more things I'd like to do before getting a test version out, but it should be soon.
  • I was wondering about sticking more closely to a sliding window/moving average value for the pause that can be settled on for a given page as Landmarks adapts to it. That could be pretty cool, though I don't think it would improve performance much more than the above, so whilst it may be an interesting learning exercise, it doesn't seem urgent.

@matatk matatk added this to the 2.1.1 milestone Jan 8, 2018
@matatk
Copy link
Owner

matatk commented Jan 9, 2018

Just a note that #131 ought to be solved before the solution to this issue is released (though it has no real side-effects, it needs tidying up) and I have made some progress on how to address #131 recently.

matatk added a commit that referenced this issue Jan 12, 2018
* Remove magic numbers in all setTimeout() calls.
* If landmarks might be out-of-date, re-scan when using the button or
  keyboard shortcuts.
* Make injected landmarks test page less verbose.
@matatk
Copy link
Owner

matatk commented Jan 12, 2018

Think I've completed everything that needs to be in order to release 2.1.1. Before I do, I made a test version for you to try. Please let me know what you think. The changes are as described above.

@MarcusJTWhich
Copy link
Author

Hi @matatk, sounds like you've made great progress indeed and I'd love to try the extension but I get the error "Failed - Forbidden" when I try to install it, alas it doesn't say anything else.

@matatk
Copy link
Owner

matatk commented Jan 12, 2018

Hmmm, I can't reproduce that; I've even had three different versions of the extension installed at once. Exactly when do you get this error—what did you click on to make it appear, and if there is any more text, can you copy and paste it here? I have a feeling this may be a Chrome issue rather than relating to Landmarks, as it seems to affect other people too. The main suggestion appears to be signing out and back in (though not sure if that means to Chrome, or to Google, just on that web page—I'm not signed in to Chrome at all).

I did set this test version of the extension to "unlisted" which means anyone with the link is meant to be able to access it.

If you can ensure that it would not contain any personal information, perhaps you could attach a screen-grab?

matatk added a commit that referenced this issue Jan 18, 2018
* Remove magic numbers in all setTimeout() calls.
* If landmarks might be out-of-date, re-scan when using the button or
  keyboard shortcuts.
* Make injected landmarks test page less verbose.
@matatk matatk closed this as completed in ccbe453 Jan 19, 2018
@matatk
Copy link
Owner

matatk commented Jan 19, 2018

This has been closed as I've merged and am about to release the code, but I would be interested in your feedback on it when you get chance to try it out, and if we need to make tweaks/changes, then that can be done in another minor release.

@MarcusJTWhich
Copy link
Author

MarcusJTWhich commented Jan 19, 2018

Sorry I haven't been responsive, I had been meaning to reply but have been crazy busy at work. I'll certainly try the new release and let you know either way! 👍

@matatk
Copy link
Owner

matatk commented Jan 19, 2018

No worries. I have been testing it for a while and given the performance improvements and that there was another fix I wanted to get out (a keyboard shortcut for opening the pop-up) I thought best to give it a go—can always make further improvements in future if needed.

Whenever you have the time, if you do have any feedback, you can let me know here. Thanks for reporting this bug (and the interesting links).

@MarcusJTWhich
Copy link
Author

I've finally got round to giving it ago and I can now use Google Sheets/Docs/Slides without any performance issues so as far as I'm concerned it's fixed, thanks very much! ^_^

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

No branches or pull requests

2 participants