Bugzilla

(In reply to Dão Gottwald [::dao] from comment #35)
> (In reply to :Gijs (he/him) from comment #34)
> > (In reply to Dão Gottwald [::dao] from comment #32)
> > > My understanding was that `updateTitlebar` itself blocked part of tab switching
> > 
> > No, none of the calls to `updateTitlebar` await the promise except the webext one (which I don't think gets hit from the test), so the promise resolution in the test is already (ie pre-patch) not dependent on `updateTitlebar` completing. It's just that we spend cycles, while waiting for other work, and as a result that other work takes longer to complete.
> 
> Okay, so the call doesn't immediately block the call site, but that still doesn't seem to mean that repeated calls are the problem. Based on what you're saying now it seems that we could just delay the work without tracking repeated calls. It still doesn't seem to me that the repeated call scenario is real enough to worry about.

Right, the repeated calls probably aren't the issue - that's just what I thought before I looked at the test. I guess from comment #15 it just seemed like another thing we should avoid if the title update is expensive - if the user switches tabs 3 times quickly (e.g. from hitting ctrl-tab repeatedly), we don't need 3 title updates, one will do...

But idle callbacks make some attempt to avoid doing the work while we're still waiting for the rest of the tab switch to complete, which `dispatchToMainThread` doesn't, which means I'm unconvinced that doing that really helps here. It might help the numbers reported by this test a bit, depending on the phase of the moon etc. in terms of when that task gets scheduled vs. when the other bits of the tab switch get scheduled. But it won't make a difference for users - things will still be slower, on the whole, because fluent has meant we now just do more work here than the "dumb" string concatenation that was there before bug . In fact, similar things may be true for `requestIdleCallback`, though the tabswitch itself (in terms of showing the new content and updating the tabstrip) is probably more responsive in that scenario vs. using `dispatchToMainThread` - but the trade-off is that if the titlebar is visible, there'll be visible slowness there, and even without the titlebar visible, on Windows the same slowness may be visible in the taskbar etc.

(In reply to Dão Gottwald [::dao] from comment #33)
> Or stop using fluent I guess. I haven't looked at all the pros and cons so I don't have an opinion on this at this stage.

This seems like the best option to me, unless we're convinced we actually need fluent's features for this string, in which case we probably want to come up with some way of making the parameter passing for the string less slow. I guess we could use `formatValue` and "manually" assign the result, which should reduce the amount of JSON marshalling that happens? Unsure if that'd be enough to fix the slowness here.

(In reply to Dão Gottwald [::dao] from comment #35)
> (In reply to :Gijs (he/him) from comment #34)
> > (In reply to Dão Gottwald [::dao] from comment #32)
> > > My understanding was that `updateTitlebar` itself blocked part of tab switching
> > 
> > No, none of the calls to `updateTitlebar` await the promise except the webext one (which I don't think gets hit from the test), so the promise resolution in the test is already (ie pre-patch) not dependent on `updateTitlebar` completing. It's just that we spend cycles, while waiting for other work, and as a result that other work takes longer to complete.
> 
> Okay, so the call doesn't immediately block the call site, but that still doesn't seem to mean that repeated calls are the problem. Based on what you're saying now it seems that we could just delay the work without tracking repeated calls. It still doesn't seem to me that the repeated call scenario is real enough to worry about.

Right, the repeated calls probably aren't the issue - that's just what I thought before I looked at the test. I guess from comment #15 it just seemed like another thing we should avoid if the title update is expensive - if the user switches tabs 3 times quickly (e.g. from hitting ctrl-tab repeatedly), we don't need 3 title updates, one will do...

But idle callbacks make some attempt to avoid doing the work while we're still waiting for the rest of the tab switch to complete, which `dispatchToMainThread` doesn't, which means I'm unconvinced that doing that really helps here. It might help the numbers reported by this test a bit, depending on the phase of the moon etc. in terms of when that task gets scheduled vs. when the other bits of the tab switch get scheduled. But it won't make a difference for users - things will still be slower, on the whole, because fluent has meant we now just do more work here than the "dumb" string concatenation that was there before bug 1591328. In fact, similar things may be true for `requestIdleCallback`, though the tabswitch itself (in terms of showing the new content and updating the tabstrip) is probably more responsive in that scenario vs. using `dispatchToMainThread` - but the trade-off is that if the titlebar is visible, there'll be visible slowness there, and even without the titlebar visible, on Windows the same slowness may be visible in the taskbar etc.

(In reply to Dão Gottwald [::dao] from comment #33)
> Or stop using fluent I guess. I haven't looked at all the pros and cons so I don't have an opinion on this at this stage.

This seems like the best option to me, unless we're convinced we actually need fluent's features for this string, in which case we probably want to come up with some way of making the parameter passing for the string less slow. I guess we could use `formatValue` and "manually" assign the result, which should reduce the amount of JSON marshalling that happens? Unsure if that'd be enough to fix the slowness here.