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

[special performance][upwards compatibility] Construct our (combined) mutation observer/s individually depending settings? #2276

Open
ImprovedTube opened this issue May 16, 2024 · 5 comments

Comments

@ImprovedTube
Copy link
Member

ImprovedTube commented May 16, 2024

"Features can have their own mutation observer each" (like this:)

if (ImprovedTube.storage.channel_default_tab && ImprovedTube.storage.channel_default_tab !== '/' ) {
new MutationObserver(function (mutationList) {
for (var i = 0, l = mutationList.length; i < l; i++) {
var mutation = mutationList[i];
if (mutation.type === 'attributes') {
ImprovedTube.channelDefaultTab(mutation.target);
}
}

And we have one combined observer processing the DOM once, and then also catching possible unexpected changes of the DOM since maybe features need to run again depending what element might change by youtube's js. So this runs a lot even without checking changes of attributes and content. (Yet even a combined observer could be constructed individually depending user settings. The average user might has only 10 in 180 features enabled. )

ImprovedTube.observer = new MutationObserver(function (mutationList) {
for (var i = 0, l = mutationList.length; i < l; i++) {
var mutation = mutationList[i];
if (mutation.type === 'childList') {
for (var j = 0, k = mutation.addedNodes.length; j < k; j++) {
ImprovedTube.childHandler(mutation.addedNodes[j]);
}
for (const node of mutation.removedNodes){
if(node.nodeName === 'BUTTON' && node.id === 'it-popup-playlist-button') ImprovedTube.playlistPopupUpdate();
}
}
if (mutation.target && mutation.target.id === 'owner-sub-count') {
if (name === 'A') {
if (node.href) {
this.channelDefaultTab(node);
if (this.storage.blocklist_activate && node.classList.contains('ytd-thumbnail')) {



( the commented out code functions.js 20-33 & init.js 30-54 are just a little optimization to be tested once more)

@raszpl
Copy link
Contributor

raszpl commented May 17, 2024

new MutationObserver(function (mutationList) {

this never executes because its being called before storage loads. This is when you broke it c7695ff#r142127013

This mutation observer listens (well, listened until you broke it :P) to a very specific thing attributeFilter: ['href'],, Im not sure what its supposed to be doing, but its very cheap to run.

Combining all mutation observers into one colossus targeting document.documentElement is a bad idea :( it triggers on everything.
This is why YT feels little sluggish with Extension loaded, this

ImprovedTube.childHandler = function (node) {
is the worst thing ever :)
Currently on channel page ytElementsHandler runs ~2000 times before first thumbnail is even inserted into the page, and keeps running for another ~20000 times before last thumbnail is added.

Just to drive point home adding ImprovedTube.counter = 0; on top of /js%26css/web-accessible/functions.js and

	} else {
						console.log(++ImprovedTube.counter);
	}

here


gives us ~24000 times ytElementsHandler was run without any reason, + another 12000 it matched
} else if (document.documentElement.dataset.pageType === 'video') {

... then again all this pointless calling of ytElementsHandler combined steals "only" ~30ms for average Video page, thats two frames of screen refresh, not the end of the world in html land.

In fact Instead of combining its the other way around. Optimally there should be multiple individual mutation observers listening only on specific target elements. I think this might be what you meant by "construct". So instead of

if (node.href) {
this.channelDefaultTab(node);
if (this.storage.blocklist_activate && node.classList.contains('ytd-thumbnail')) {
this.blocklist('video', node);

testing every link for 'ytd-thumbnail' class to run blocklist there would be dedicated mutation observer added only after '#items #contents' suggestion sidebar was added to the page and listening to only changes inside '#items #contents' target.
Problem with this solution is fragility, every time YT does the slightest change (order it injects content) things could/would break.

The lightest way possible is waiting until YT loads everything and then scanning once for things we are interested in, this should handle 90% of what ImprovedTube.ytElementsHandler is scanning for on every element on every mutation. Only after this one big scan would ytElementsHandler mutation observer be started.

@ImprovedTube
Copy link
Member Author

ImprovedTube commented May 18, 2024

hi! @raszpl!
we could avoid to check features that aren't enabled. (Thats what i meant)

waiting until YT loads

-and avoid depth of recursion at first, unless it will be of interest somewhere later/cheaper

Yes, to be (much) future-proof would be worth some ms 😊 (/not hard to have the confidence to add 0.5% to the total time YouTube loads anyways.), yet we aren't using the Observer much like that and these edits making guesses should be aware how often these lines might run: https://github.com/code-charity/youtube/blob/96879c6e2ce0321d68b1a27f36251c9a92beba44/js%26css/web-accessible/functions.js#L171-220

else if (name === 'YTD-TOGGLE-BUTTON-RENDERER' || name === 'YTD-PLAYLIST-LOOP-BUTTON-RENDERER') {
//can be precise previously node.parentComponent & node.parentComponent.parentComponent
if (node.closest("YTD-MENU-RENDERER")
&& node.closest("YTD-PLAYLIST-PANEL-RENDERER")) {
var index = Array.prototype.indexOf.call(node.parentNode.children, node);
if (index === 0) {
if (this.storage.playlist_reverse === true) {
//can be precise:
try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode;}
catch{try{this.elements.playlist.actions = node;}catch{}}
}
}
}
}
this.playlistReverse();
} else if (index === 1) {
this.elements.playlist.shuffle_button = node;
this.playlistShuffle();
if (this.storage.playlist_reverse === true) {
//can be precise:
try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode;}
catch{try{this.elements.playlist.actions = node;}catch{}}
}
}
}
}
this.playlistReverse();

And it doesn't watch attributes & characterData yet. But one could suppress many elements instantly to clean, ( thats after adjusting JS before DOM is ready or after intercepting http request with firefox w3c/webextensions#506) )


never executes

channel_default_tab() is working 😅


after '#items #contents' suggestion sidebar was added

blockable items are also called #dismissible. In #secondary #related, ytd-search, ytd-two-column-browse-results-renderer, and the related videos grid


( repo-history 2020 https://github.com/code-charity/youtube/blob/2fc9341cb2066240a3b6dfc2da763feb06dec5e1/src/youtube/js/mutations.js 2021 https://raw.githubusercontent.com/code-charity/youtube/2bba2fe772dc8a7abdc06cfe8475bc88b6c5ed56/youtube-scripts.js https://raw.githubusercontent.com/code-charity/youtube/2bba2fe772dc8a7abdc06cfe8475bc88b6c5ed56/content-scripts.js )

@raszpl
Copy link
Contributor

raszpl commented Jun 30, 2024

else if (name === 'YTD-TOGGLE-BUTTON-RENDERER' || name === 'YTD-PLAYLIST-LOOP-BUTTON-RENDERER') {
//can be precise previously node.parentComponent & node.parentComponent.parentComponent
if (node.closest("YTD-MENU-RENDERER")
&& node.closest("YTD-PLAYLIST-PANEL-RENDERER")) {
var index = Array.prototype.indexOf.call(node.parentNode.children, node);
if (index === 0) {
if (this.storage.playlist_reverse === true) {
//can be precise:
try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode;}
catch{try{this.elements.playlist.actions = node;}catch{}}
}
}
}
}
this.playlistReverse();
} else if (index === 1) {
this.elements.playlist.shuffle_button = node;
this.playlistShuffle();
if (this.storage.playlist_reverse === true) {
//can be precise:
try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode.parentNode;}
catch{try{this.elements.playlist.actions = node.parentNode;}
catch{try{this.elements.playlist.actions = node;}catch{}}
}
}
}
}
this.playlistReverse();

And it doesn't watch attributes & characterData.

that function is so broken its not even funny, what is this clown car of a try catch chain? :) the first one will always match because every node this deep has 4 childnodes :-]
Digging deeper this was
1 Reverse playlist button no longer functioning.
#1544

2 makes some sense but is bad because slow
2ddc797

3 makes zero sense
77f7be2#diff-566da1f8c3eefe7102e8d0bc6ea76a35b37f82d0828b9d8c3a137bdce6fa7463

  • loading playlist/video with playlist makes this call multiple times per playlist item
  • it calls this.playlistReverse() on both buttons so x2 x number of playlist items x number of nodes mutated per playlist item inserted, easily couple hundred calls just to add ONE button :D
  • whats up with this index = Array.prototype.indexOf.call(node.parentNode.children, node); ? we dont need index to know which button is which.

Speaking of Playlists, Playlist menu has those 4:

Reverse
Repeat
Shuffle
Popup player

but:
Reverse and Popup player are buttons we are adding
Repeat and Shuffle are toggles we are switching/forcing
Those should be in separate sub categories.

I have this semi fixed locally together with some optimizations I will be submitting later.

And one could suppress many elements instantly to clean,

You might think you can, but no:
-element changed and you want to apply some function to it again (overriding it)
-next page loaded, need all listeners again
-more content streamed in (infinite scroll)
-there is more than one element using same node name we apply same function() to
-etc

never executes

channel_default_tab() is working 😅

because its also being called in

this.channelDefaultTab(node);

making this whole another separate mutation observer do useless double work :(

after '#items #contents' suggestion sidebar was added

blockable items are also called #dismissible. In #secondary #related, ytd-search, ytd-two-column-browse-results-renderer, and the related videos grid

after running some monitoring blocklist doenst cost us much, YT likes to add thumbnail A nodes and then keep reusing them over and over

( repo-history 2020 https://raw.githubusercontent.com/code-charity/youtube/2bba2fe772dc8a7abdc06cfe8475bc88b6c5ed56/content-scripts.js )

Hey this looks familiar

function attributes(items) {
    var whitelist = {
        'youtube-home-page': true,
        'remove-related-search-results': true,
        'squared-user-images': true,
        'hide-animated-thumbnails': true,
        'header-position': true,
        'header-improve-logo': true,
        'header-hide-right-buttons': true,
        'header-hide-country-code': true,
        'player-hide-annotations': true,
        'player-hide-cards': true,
        'player-show-cards-on-mouse-hover': true,
        'player-size': true,
        'player-color': true,
        'player-transparent-background': true,
        'player-hide-endscreen': true,
        'hide-scroll-for-details': true,
        'always-show-progress-bar': true,
        'hide-details': true,
        'hide-views-count': true,
        'hide-date': true,
        'hide-share-button': true,
        'hide-save-button': true,
        'hide-more-button': true,
        'likes': true,
        'red-dislike-button': true,
        'description': true,
        'livechat': true,
        'hide-playlist': true,
        'related-videos': true,
        'comments': true,
        'hide-footer': true,
        'night-theme': true,
        'dawn-theme': true,
        'sunset-theme': true,
        'desert-theme': true,
        'plain-theme': true,
        'black-theme': true,
        'player-crop-chapter-titles': true,
        'player-ads': true,
        'improvedtube-youtube-icon': true
    };

    for (var key in items) {
        var attribute = key.replace(/_/g, '-');

        if (whitelist.hasOwnProperty(attribute)) {
            document.documentElement.setAttribute('it-' + attribute, items[key]);

its my #2197 :]

@ImprovedTube
Copy link
Member Author

ImprovedTube commented Sep 20, 2024

that function is so broken its not even funny, what is this clown car of a try catch chain? :) the first one will always match because every node this deep has 4 childnodes :-]
makes zero sense
77f7be2#diff-566da1f8c3eefe7102e8d0bc6ea76a35b37f82d0828b9d8c3a137bdce6fa7463

...A quick test from when the line looking for four caused an error.
Youtube's DOM used to make extra copies of these elements under some conditions.
( if (name === 'YTD-TOGGLE-BUTTON-RENDERER' || name === 'YTD-PLAYLIST-LOOP-BUTTON-RENDERER') )
(hi @raszpl)

@ImprovedTube
Copy link
Member Author

I have this semi fixed locally together with some optimizations I will be submitting later.

will you / we?

@ImprovedTube ImprovedTube changed the title [performance:] construct our combined mutation observer individually when settings are changed? [special performance?] Construct our (combined) mutation observer/s individually depending settings? Nov 3, 2024
@ImprovedTube ImprovedTube changed the title [special performance?] Construct our (combined) mutation observer/s individually depending settings? [special performance][upwards compatibility] Construct our (combined) mutation observer/s individually depending settings? Nov 3, 2024
@ImprovedTube ImprovedTube removed their assignment Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants