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

[adblocker-electron] Application is extremely lagged (memory leak) #3420

Closed
JellyBrick opened this issue Sep 4, 2023 · 9 comments · Fixed by #3441
Closed

[adblocker-electron] Application is extremely lagged (memory leak) #3420

JellyBrick opened this issue Sep 4, 2023 · 9 comments · Fixed by #3441

Comments

@JellyBrick
Copy link

JellyBrick commented Sep 4, 2023

7782d67
Performance regression occurred after that commit.

(This issue does not occur in versions 1.26.3 and earlier)

Code

https:/organization/youtube-music-next/blob/feat/typescript/plugins/adblocker/blocker.ts

Related issues

th-ch/youtube-music#1187
th-ch/youtube-music#1208
th-ch/youtube-music#1212

@JellyBrick JellyBrick changed the title Extremely laggy when using adblocker-electron [adblocker-electron] Application is extremely lagged (memory leak) Sep 4, 2023
@remusao
Copy link
Collaborator

remusao commented Sep 16, 2023

Hi @JellyBrick,

Thanks for reaching out and sorry for the delay in answering. This looks like a serious issue and multiple people have already encountered it. At the moment I have limited bandwidth (and limited knowledge about Electron stack) so I am not able to investigate it myself. I'm gonna ping a few people who are familiar with it in the hope that we can get some clarity regarding the root cause of the slow-down.

@DrRoot-github since you opened the PR and are most likely the person with most insights in the change, would you have some availability to look into it? I believe @Jelmerro also investigated as part of the issue on Vieb-side.

Could this issue be caused by using sendSync from the page? Alternatively, maybe we are returning too much on updates (e.g. maybe we wrongly return scriplets or too many selectors).

I hope that helps,

@DrRoot-github
Copy link
Contributor

I'll try to investigate a bit more, so please wait a little longer. If possible, it would be helpful if you could provide specific instructions on how to reproduce it. I think I might be the culprit, seriously sorry.

@remusao
Copy link
Collaborator

remusao commented Sep 17, 2023

@DrRoot-github No worries at all, I think I found the issue and created a PR to fix it here: #3441 Would you have time to review it and maybe test that the blocking of ads in YouTube didn't regress? (from some local testing it seems fine).

Cc @Jelmerro once reviewed we can publish an update of the adblocker package (as early as today if anyone has time to review the changeset).

@KunaPrime
Copy link

KunaPrime commented Sep 17, 2023

I'm user of Vieb and @Jelmerro have provided me a patch with a fix (i assume it is based on #3441) and i can confirm that regression is gone.

@remusao
Copy link
Collaborator

remusao commented Sep 17, 2023

Thank you @KunaPrime and @Jelmerro, I have merged the PR and a new release will be available shortly.

@remusao
Copy link
Collaborator

remusao commented Sep 17, 2023

The release pipeline got broken by some upstream dependency updates. Until it gets fixed, I have pushed a canary release manually which you can try out: https://www.npmjs.com/package/@cliqz/adblocker-electron/v/1.26.7--canary.50688cf.0

JellyBrick added a commit to organization/youtube-music-next that referenced this issue Sep 20, 2023
@JellyBrick
Copy link
Author

JellyBrick commented Oct 17, 2023

This issue is still reproducible in v1.26.8

@remusao
Copy link
Collaborator

remusao commented Oct 17, 2023

Could you provide some more details about this please?

@JellyBrick
Copy link
Author

JellyBrick commented Oct 18, 2023

I found the problem that the responsiveness of the webpage has decreased over time.

(Electron 27.0.0)

Tested webpage: https://music.youtube.com

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

Successfully merging a pull request may close this issue.

4 participants