Closed Bug 1185567 Opened 9 years ago Closed 2 years ago

Apply the delayprefsave functionality to the base <preference> binding, making the functionality opt-out

Categories

(Firefox :: Settings UI, defect)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: quicksaver, Unassigned)

References

Details

Attachments

(2 files)

Bug 1008169 presents the most noticeable case of a more general problem, and I think the solution applied should have been more generalized.

In my main profile, it can take between a quarter of a second to a full second to toggle a single checkbox (one that has no extra listeners to apply the change, just flipping its boolean value).

I've attached an xpi I quickly whipped up with a preferences panel where you can easily notice the difference between the immediate method and the delayed method (with 'delayprefsave' attribute). Just install, open add-ons manager and open its preferences tab (chrome://prefspeedtest/content/utils/preferences.xul#test, ignore the about pane).

Also, IMO 1000ms delay is a bit much, I would have gone with 250ms. 100ms is probably underdoing it though.

CCing Gijs and Jared who were assignee and reviewer in that bug.
By "it can take [...] a second" I meant the time during which the UI locks up until it finishes toggling the pref. Obviously the delayed method "takes longer" as intended, but it doesn't lock up the UI.
I don't see an attached XPI on this bug. Can you try to reupload it?

Do you have any ideas as to why the pause may be happening?
Flags: needinfo?(quicksaver)
Attached file Prefs Speed Test XPI
Sorry about that, I was sure I had attached the file.

I tried figuring out where the delay came from, but I honestly can't tell. It doesn't seem to be from the pane's userChangedValue() method or anything downstream (but I could be wrong, I only quickly tried pasting that code - and code from subsequent methods - by hand into the console to simulate a pref change but didn't see any noticeable delay anywhere).
Flags: needinfo?(quicksaver)
I forgot to mention, that XPI only works in FF41 and above, as it assumes the patch from bug 1008169 has been applied.
Oh, so this add-on just visualizes the difference between the setTimeout(function(){}, 1000), setTimeout(function(){}, 250), and setTimeout(function(){}, 100)?

Yeah, 1000ms may be a while, but dropping this down to 250ms or 100ms won't fix the issue that you're facing where it may take up to a second to flip a normal checkbox.

I think the slowdown with the font changes depends on what tabs you have open and how the font changes will affect all of those tabs to re-layout their contents.
Screencast might explain better what I mean?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Oh, so this add-on just visualizes the difference between the
> setTimeout(function(){}, 1000), setTimeout(function(){}, 250), and
> setTimeout(function(){}, 100)?

I realize I should have been more specific. It also compares that yeah, but not the main point I tried to make.

(BTW it's not a setTimeout, it's the exact same method as applied in bug 1008169 with the deferred task, except with different delays. Let me know if you'd like me to point you to the appropriate files that are relevant, although I don't think that'll be necessary.)

Maybe the screencast will help, this is what I do in the beginning of the screencast:
- click in any of the radio boxes to focus them
- hold the "down" key to cycle through them

If I do this in the Immediate column it will seem like it locks up while I keep the key pressed (my guess because it has to update the preference in every step), whereas if I do it in any of the delayed columns I can actually see the point mark jumping through the options.

Same thing for the menulists, if I focus the Immediate one and do the same, it will seem like it locks up while I keep the down button pressed, but I can actually see the selected value change on screen if I do it in any of the delayed ones.

Same thing for typing in the textbox and holding down a key to repeat a character, holding down the mouse button to increase or decrease the numbox, sliding the scale, etc. It's the responsiveness of the items themselves that I'm focusing on.

The other items also suffer from this, except it's more subtle because they're all "single actions", so they're easier to dismiss as "something happening backstage".

I.e. clicking the Immediate checkbox takes longer to mark/unmark that same checkbox than it does for the others. It's not the time it takes for the actual preference to flip, but the time between the actual mouse click and the check mark (dis)appearing. There's a noticeable (and quite large) fraction of a second when the browser becomes unresponsive after clicking it, which doesn't happen for any of the delayed columns. In other words, 

> Yeah, 1000ms may be a while, but dropping this down to 250ms or 100ms won't
> fix the issue that you're facing where it may take up to a second to flip a
> normal checkbox.

That was a point on its own, sort of a tangent for which I should open a new bug but wanted to know your opinions first. With those different delays I meant to show that there's no need to go so far up to 1000ms, as responsiveness will stay the same anywhere between 250ms to 1000ms (at least for me, I guess that would depend on the system?).

A larger delay just means the user will need to wait that much longer until he sees the change he's going for.

> I think the slowdown with the font changes depends on what tabs you have
> open and how the font changes will affect all of those tabs to re-layout
> their contents.

Hmm, I definitely didn't read that bug like that. I thought the problem in there was inherent to the preferences nodes themselves, like what I'm trying to show here. And now that I think about it, the patch in bug 1008169 just so happened to fix both problems for the those font selectors.
A quick example in firefox's own preferences is the homepage textbox. If I focus it and hold down any key, it will seem like it locks up while it inserts all the repeated characters, like it does in my screencast.
Ok, I think I understand what you're asking for now. I've updated the summary to make it more clear how we could fix this.

As well, we could probably drop it to 250 or 100ms. The balance there is if it takes over 100ms to actually persist, and we have it set to delay at 100ms, then we can get caught in a tight loop there.
Summary: Preferences are sluggish to toggle or modify → Apply the delayprefsave functionality to the base <preference> binding, making the functionality opt-out
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Ok, I think I understand what you're asking for now. I've updated the
> summary to make it more clear how we could fix this.

Yes, it's exactly that. :) I'm beginning to think I have a tendency to over-complicate things, thanks for putting up with it.

> As well, we could probably drop it to 250 or 100ms. The balance there is if
> it takes over 100ms to actually persist, and we have it set to delay at
> 100ms, then we can get caught in a tight loop there.

Would this work: setting a ._savingPref property as a flag on the preference, and only re-arm the deferred task according to it?
Alternative:

opt-out attribute still called "delayprefsave" with values:
=== "false" to save pref immediately;
== any number to change the delay (going through a parseInt() obviously).

Delay could default to 250ms which should be acceptable for most types of preferences, and the font seletors (bug 1008169) could still keep their 1000ms delay if that's a better value for them because of the many-tabs-open-having-to-update-because-of-it issue.
(In reply to Luís Miguel [:Quicksaver] from comment #9)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > Ok, I think I understand what you're asking for now. I've updated the
> > summary to make it more clear how we could fix this.
> 
> Yes, it's exactly that. :) I'm beginning to think I have a tendency to
> over-complicate things, thanks for putting up with it.
> 
> > As well, we could probably drop it to 250 or 100ms. The balance there is if
> > it takes over 100ms to actually persist, and we have it set to delay at
> > 100ms, then we can get caught in a tight loop there.
> 
> Would this work: setting a ._savingPref property as a flag on the
> preference, and only re-arm the deferred task according to it?

Shouldn't we just delay the task until after the save is done? I'm pretty sure that the save is happening on the main thread though, because things are horrible, which means, AIUI, that we can't be re-arming until the save has finished anyway... :-)

Speaking of which... does this code currently flush the pref file? Because I think it does. In which case we're basically doing some main-thread disk IO every time you type and such, which is very obviously very sad. Taking out that IO and using bug 789945 seems like a more sensible option in that case, though a short-term workaround. Jared, can you doublecheck that?
Flags: needinfo?(jaws)
(In reply to :Gijs Kruitbosch from comment #11)
> Speaking of which... does this code currently flush the pref file? Because I
> think it does. In which case we're basically doing some main-thread disk IO
> every time you type and such, which is very obviously very sad. Taking out
> that IO and using bug 789945 seems like a more sensible option in that case,
> though a short-term workaround. Jared, can you doublecheck that?

I spent a short amount of time looking in to this and it does look like we are flushing the prefs. Each pref change makes its way to the valueFromPreferences setter which calls `this.preferences.service.savePrefFile(null);` as long as it's not batching saves.

Bug 789945 looks like the right path here.
Flags: needinfo?(jaws)

The profiler doesn't show anything meaningfully problematic happening here anymore, so I think we're good here.

Severity: normal → S3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: