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

Perfomance: allow to set the onUpdateUI timeout using a hidden pref (… #2589

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

Conversation

Defman21
Copy link
Contributor

@Defman21 Defman21 commented May 5, 2017

…onUpdateUI_timeout); fixed #2470

Signed-off-by: Defman21 i@defman.me

…onUpdateUI_timeout); fixed Komodo#2470

Signed-off-by: Defman21 <i@defman.me>
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would invoke getLong across xpcom every time updateUI is called, which is bad for performance. If we are to make a pref for this the pref needs to be stored (cached) and updated as needed (via pref observers).

@Defman21
Copy link
Contributor Author

Defman21 commented May 5, 2017

I'll look into that.

@Defman21
Copy link
Contributor Author

Defman21 commented May 5, 2017

Now ko/prefs is being called only once per buffer opening to get the pref value (i don't know how to call an observer immediately to set the initial value) . Each buffer has its own pref observer to update the onUpdateUI_timeout attribute.

Signed-off-by: Defman21 <i@defman.me>
Signed-off-by: Defman21 <i@defman.me>
@Defman21
Copy link
Contributor Author

Defman21 commented May 5, 2017

I'll do a fixup/rebase once you'll review my changes :)
One require call per buffer opening is not a big deal imo (<view>.prefs will return it without requiring again though)

Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only creating the observer and not removing it. Which could lead to lingering observers and a minor memory leak. But seeing this I'm kind of second guessing whether to use an observer at all. I don't like the idea of having an observer for each buffer, but there is no better solution that I can see.

Given that this is a hidden pref I think we should just remove the observer part altogether. If someone wants to change this pref they'll have to restart Komodo or reopen their buffers. This pref will never be exposed in the UI.

@Defman21
Copy link
Contributor Author

Defman21 commented May 5, 2017

I agree.

Signed-off-by: Defman21 <i@defman.me>
…e view.prefs instead of require

Signed-off-by: Defman21 <i@defman.me>
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.

Allow to set the onUpdateUI timeout value
2 participants