Stylish CPU/battery abuse on AJAX/DOM-heavy sites since 1.9.4

[ghost]

New issue to track this separately.

Problem:

• 1.9.4 started listening to DOMSubtreeModified and calling checkStyles every time.
• That event triggers extremely rapidly on modern sites that use dynamic DOM manipulation. For example, go to YouTube and type this in your Javascript Console quickly after a video page has begun loading: document.addEventListener("DOMSubtreeModified", function(event) { console.log("- tree modified"); });. And the situation is much worse on other sites.
• Running checkStyles will loop every time through all page styles and use "getElementById" for each injected style's ID. And inject it if missing. This is extra, useless work that happens repeatedly on every webpage and slows down DOM manipulation, uses extra CPU and therefore causes more battery drain.

Needed:

• Optimize checkStyles so that it is synchronized and only triggers once for every huge group of DOMSubtreeModified calls.

Solution 1:

var domModifiedTimer = null;
d.addEventListener("DOMSubtreeModified", function(event) {
    if (domModifiedTimer) {
        clearTimeout(domModifiedTimer);
    }
    domModifiedTimer = setTimeout([function here], 250);
});

// for [function here], use a reference to a named/static function
// (which takes zero parameters), instead of an anonymous callback,
// to avoid creating a new anonymous function 1000s of times per webpage.

What this does:

• Imagine that the user is on Facebook.com and they click on a link, which runs some Ajax query and then uses Javascript to create 200 DOM elements. That's 200 calls to DOMSubtreeModified!
• The timer has a 250 millisecond (0.25 second) timeout so if those 200 elements are inserted super fast, the timer will clear itself 199 times and then the 200th time it will actually run the Stylish code.
• That's a massive CPU saving, compared to running the whole "checkStyles" code 200 times!
• The timeouts will not "stack in inactive tabs and all execute at once" (which @350d worried about last time we spoke about this), since clearTimeout() makes sure that only 1 timer will ever exist at a time.
• So using a timer is a simple way to fix the problem, and it would work well.

Solution 2:

Synchronize the "checkStyles" to happen only before the screen is about to repaint.

• requestAnimationFrame() tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint.
• This is a bad solution. For many reasons. First of all, you would need to synchronize your usage of requestAnimationFrame to never call it twice (while you've already called it once and are waiting for the next frame), otherwise you'd register your callback to happen multiple times the next time the screen redraws. So you'd still end up with an extra variable to keep track of the state of whether we've already requested an animation frame (which is checked before calling requestAnimationFrame, so that it never calls it again while a call is already waiting). And that extra variable may possibly get bugged, leading to no more calls happening at all.
• Secondly, most websites are NOT PROGRAMMED PROPERLY. They manipulate the DOM in a way that causes constant repaints (by manipulating visible DOM trees; as opposed to building a sub-tree detached from the DOM and then inserting it all at once to only trigger a single animation frame/screen redraw), which means that there would be ZERO performance gain by using requestAnimationFrame on most websites.
• I do not recommend this method. At all. It's a "nice" idea in theory to synchronize to frames, but it doesn't work well for most websites and wouldn't give any performance gain.

Anyway, if anyone has any better ideas for how to optimize the checkStyles() penalty on DOMSubtreeModified, please let us know!

@350d added the DOMSubtreeModified code because some sites insert DOM elements later than the initial load, and they aren't caught/styled on the first Stylish run (at page load). So he needed to add that constant checkStyles call. Now we just need a way to optimize it...

[350d]

@SteveJobzniak You can open Facebook.com and put this in console to check for how long it takes to make 1M getElementById:

var t = performance.now();
for (var i = 0; i < 1000000; i++) var x = document.getElementById("pagelet_dock");
document.title = Math.round((performance.now() - t)*10000)/1000 + 'ms';

i have ~290ms
Where is the CPU abuse in case of using single or 10 getElementById on every dom update?

[350d]

@SteveJobzniak I'm working on better method - DOMNodeRemoved.
I can constantly check for removed elemets ids and only in case of styles removed - re-inject styles.

[350d]

Another test with parentElement check, 2.5 times faster than getElementById :

var x = document.getElementById("pagelet_dock");
var t = performance.now();
for (var i = 0; i < 1000000; i++) var y = x.parentElement;
document.title = Math.round((performance.now() - t)*10000)/1000 + 'ms';

~100ms

[ghost]

@350d Thanks for researching this. The speed penalty was lower than I thought, but it's still annoying to have a browser extension "run" its own code constantly whenever the page does (very common) DOM work, even when the extension shouldn't have to do work. So it's good that you'll improve it. And your ideas for improvement sound very good!

DOMNodeRemoved = Much, muuuch rarer than DOMSubtreeModified, so this cuts down the calls a lot! People add/modify nodes all the time, but they very rarely remove nodes, and usually only remove a single parent node (which kills all children), so this would be the biggest improvement of all!

parentElement = And this is faster than getElementById, as you discovered.

So I guess you'd have to add all injected style DOM nodes to an array, and then whenever a node is removed from the page you just check if all of your injected styles still have their parentElement. And since you inject styles directly onto a top-level element (you use (d.body || d.documentElement || d.head).appendChild(style, null);), we can be sure that if the style node's parent is missing then the style has been removed from the DOM.

Very good method! With that, I can safely say that Stylish is doing its best to play nicely. 👍

[350d]

@SteveJobzniak DOMSubtreeModified replaced with DOMNodeRemoved and extra check for unique conditions added in new version 1.9.7:

if (!busy && Object.size(page_styles) && event.target.localName == 'style' && event.target.id[0] == '') {}

So, single style inject will be fired only if exact style was removed

[ghost]

@350d I've now read the 1.9.7 commit. It looks excellent! Happy to see that the new event now only performs work on the page when it has a reason to check if a style has been removed. This is a very nice optimization! :) Thank you!