Inefficient setInterval usage will waste resources and slow things down

[0xdevalias]

Just came across this script, and noticed that the addblocker function uses a setInterval with a 50ms interval, that performs ~6 document selectors. That's going to lead to a lot of redundant processing, even when a youtube tab is open but not playing anything.

ChatGPT

Reviewing your userscript, several aspects stand out in terms of performance and best practices:

• Interval Usage: The script uses setInterval to repeatedly check for elements and perform actions. This can be demanding on resources, especially with short intervals (50ms and 1000ms). Consider using mutation observers or event listeners where possible to react to changes instead of polling.

• Query Selectors in Loops: The script repeatedly queries the DOM inside setInterval calls. Frequent DOM querying can be inefficient. It's better to target specific events or changes that indicate the need to query.

Not sure if these issues are related to this:

• https://github.com/TheRealJoelmatic/RemoveAdblockThing/issues/130
• https://github.com/TheRealJoelmatic/RemoveAdblockThing/issues/135
• https://github.com/TheRealJoelmatic/RemoveAdblockThing/issues/136
• https://github.com/TheRealJoelmatic/RemoveAdblockThing/issues/137

[0xdevalias]

I haven't tested this, and it could almost certainly be refined further, but here's a little mockup ChatGPT wrote based on me prompting/refining it around this:

A more efficient approach would involve checking the nature of the mutations and only running removeAds when it's likely that ads were added or modified. Here's how you can refine the addblocker function:

1. Inspect Mutations More Closely: Instead of removing all ads for every mutation, inspect the mutation records to determine if they are likely to involve ads. For example, you can check if the added nodes match the selectors for ads.

2. Selective Ad Removal: Only remove ads that are newly added to the DOM, rather than removing all ads every time.

Here's an updated version of the function that incorporates these improvements:

function addblocker() {
    const observerConfig = {
        childList: true,
        subtree: true
    };

    const adSelector = [
        '.videoAdUiSkipButton', 
        '.ytp-ad-skip-button',
        'ytd-action-companion-ad-renderer', 
        'div#root.style-scope.ytd-display-ad-renderer', 
        'div#sparkles-container.style-scope.ytd-promoted-sparkles-web-renderer',
        'div#main-container.style-scope.ytd-promoted-video-renderer',
        'ytd-in-feed-ad-layout-renderer',
        '.ytd-video-masthead-ad-v3-renderer',
        'div#player-ads.style-scope.ytd-watch-flexy',
        'div#panels.style-scope.ytd-watch-flexy',
        '.ytp-ad-skip-button-modern'
    ].join(', ');

    const adObserver = new MutationObserver(mutations => {
        mutations.forEach(mutation => {
            if (mutation.addedNodes.length) {
                mutation.addedNodes.forEach(node => {
                    if (node.matches && node.matches(adSelector)) {
                        node.remove();
                    }
                });
            }
        });
    });

    adObserver.observe(document.body, observerConfig);
}

// Call the function
addblocker();

In this revised version, the MutationObserver callback checks each added node to see if it matches the ad selectors. If it does, it removes that node specifically. This method is more targeted and efficient, as it only removes nodes that are likely to be ads, instead of removing all potential ads every time there's any mutation in the DOM.

[0xdevalias]

You could probably also do something similar for the popupRemover, though at least it only runs once per second rather than every 50ms.

Again, I didn't validate this code at all, but here's ChatGPT's suggestion:

To refine the popupRemover function for efficiency, you can apply a few key improvements:

1. Replace setInterval with MutationObserver: Similar to the addblocker function, using a MutationObserver is more efficient than continuously polling the DOM with setInterval. This way, your code responds to changes in the DOM rather than running at a fixed interval.

2. Optimize Selector Queries: Limit the number of DOM queries. If certain elements like fullScreenButton are static and don’t dynamically change, you can query them once outside of the observer callback.

3. Selective Removal: Only perform actions if the mutations involve the specific elements you are interested in.

4. Avoid Redundant DOM Manipulations: Check if an action is necessary before performing it to avoid unnecessary DOM manipulations.

Here's a revised version of the popupRemover function:

function popupRemover() {
    const observerConfig = {
        childList: true,
        subtree: true
    };

    const fullScreenButton = document.querySelector(".ytp-fullscreen-button");
    const bodyStyle = document.body.style;
    bodyStyle.setProperty('overflow-y', 'auto', 'important');

    const observer = new MutationObserver(mutations => {
        mutations.forEach(mutation => {
            if (mutation.addedNodes.length) {
                checkAndRemovePopup();
            }
        });
    });

    observer.observe(document.body, observerConfig);

    function checkAndRemovePopup() {
        const modalOverlay = document.querySelector("tp-yt-iron-overlay-backdrop");
        const popup = document.querySelector(".style-scope ytd-enforcement-message-view-model");
        const popupButton = document.getElementById("dismiss-button");

        if (modalOverlay) {
            modalOverlay.removeAttribute("opened");
            modalOverlay.remove();
        }

        if (popup) {
            if (debug) console.log("Remove Adblock Thing: Popup detected, removing...");
            popupButton?.click();
            popup.remove();
            unpausedAfterSkip = 2;

            fullScreenButton.dispatchEvent(mouseEvent);
            setTimeout(() => {
                fullScreenButton.dispatchEvent(mouseEvent);
            }, 500);

            if (debug) console.log("Remove Adblock Thing: Popup removed");
        }

        // UnPause the video if necessary
        if (unpausedAfterSkip > 0) {
            const video1 = document.querySelector("#movie_player > video.html5-main-video");
            const video2 = document.querySelector("#movie_player > .html5-video-container > video");
            unPauseVideo(video1);
            unPauseVideo(video2);
        }
    }
}

// Additional functions like unPauseVideo should be defined outside of this function.

In this version, checkAndRemovePopup is called only when there are added nodes in the DOM, which could potentially be the popups you're targeting. The function then checks for the presence of these popups and performs actions only if necessary. This approach is more efficient because it minimizes unnecessary DOM queries and operations.

[0xdevalias]

@TheRealJoelmatic Are you able to link to the PR / commit this was fixed in as context?

Edit: Skimming through the latest commits, I can see that it's still an issue in 5.3:

• https://github.com/TheRealJoelmatic/RemoveAdblockThing/commit/92871c45b05402adb05ede613a69d5fc7d811a89#diff-b0e7a0b34d8789bf8d66489af23f67dc3cf659f05940262930ae7893ec9f3b53R296

And as best I can tell, is still an issue in 5.5, which at time of writing is the latest commit:

• https://github.com/TheRealJoelmatic/RemoveAdblockThing/commit/d0370b5cc1fa5b4d741eebf45a93288bace260b9

@TheRealJoelmatic Did you actually fix this? Or just close the issue with no context? It's generally good 'open source etiquette' to at least provide some context when closing an issue.