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

Don't compute style updates if no listerners #330

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

matthewturk
Copy link
Contributor

As noted in #315, annotator can get into a situation where it uses a lot of CPU. In my cases, this happens when the CSS is very large but no windows are open that include annotation views.

This PR adds a check to the listerner code so that if there are no open views of the annotation, it does not conduct the (potentially quite expensive) concatenation of CSS styles and comparison to the previous style.

This still results in high-CPU usage if the annotation view is open and there are many many CSS files (or a few very large ones, like with embedded fonts/icons) but it's a massive improvement in overall performance/usage in the 'idle' case.

@matthewturk
Copy link
Contributor Author

I updated the PR to have a setting which turns the styleObserver on and off, as it turns out that listerners can get added and not removed in some cases. This solves my performance issues on my Vault with lots of embedded fonts/icons in the CSS.

@aladmit
Copy link
Collaborator

aladmit commented Aug 2, 2023

@matthewturk Thank you for contribution! I don't have enough free time to deep dive into plugin performance myself, that's awesome you decided to look into it 🚀

I'll check and test this change next week.

@matthewturk
Copy link
Contributor Author

I've merged with the current branch and verified that the changes still work to reduce the (idle-time) performance hit from style calculation.

@tye-singwa
Copy link

Thanks! Had same problem, compiled plugin on this branch, replaced plugin with this bugfix with option turned off. Do not see any freezes yet

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 this pull request may close these issues.

3 participants