1613507 - Set `speculative-parallel-limit` back to the real default value

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement)

Description

[Sean Feng [:sefeng211]]

network.http.speculative-parallel-limit is set to 0 in marionette and the performance team wants to set it to the real default value to 6 so that speculative connection can be used correctly.

I am happy to take the task, however I am not sure if the task is as simple as changing the number. Based on the comment, it seems that there were reasons to set to 0 (because of some leaking?). Perhaps there are some fundamental changes that are required?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For which kinds of tests do you need to change it? I can see that for performance tests we have:

https://searchfox.org/mozilla-central/source/testing/profiles/perf/user.js#72

That is used by Talos and Raptor, and shouldn't affect Marionette. Also the wording in Marionette / geckodriver we simply got from the various profiles in testing/profiles. Originally it was all added on bug 992611.

So I think that I have to understand first your use-case.

Comment 2

[Sean Feng [:sefeng211]]

Yup, we do want to change it for Talos and Raptor and I am going to open a separate bug for it.

The reason we want to change in Marionette is because this is error-prone. It took a long time for the performance team to figure speculative connection didn't work. So we want to change it unless there's a good reason to keep it.

What do you think, whimboo?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I wasn't around when the changes for bug 992611 have been landed. As such I don't know why speculative connections have been turned off. Maybe Joel or Matthew still remember?

Marionette /geckodriver run nearly all the time for pages, which are not served from localhost. As such changing this preference to the default value might have a big impact on our users. But I wouldn't be opposed in doing that, if the implications are understood and it's clear for me why it has been done that way several years ago.

Note that any performance harness which is based on or uses marionette / geckodriver might have to be modified first to support different values for this preference, or sets it to 0 by default. Then users could set every value they want. If we don't do that first I'm concerned about changes in performance measurements.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

I honestly don't remember anything, I would be curious to see if anything breaks and what the noise levels of all tests are when switching this to the default.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is not easy to do for us in terms of marionette and geckodriver. All of our users use releases of geckodriver and Firefox. So once we shipped it will be too late. In-tree we only have the webdriver tests for checks, which I don't see as enough.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

One thing we could actually do is to only change it for Marionette and leave geckodriver alone for now. That way Selenium users and BrowserTime wouldn't be affected.

Comment 7

[Sean Feng [:sefeng211]]

Ah, so both Marionette and Geckodriver sets to 0.

Hmm, I think changing it only for Marionette doesn't fit our initial needs, because the tools we use to test performance use geckodriver, and the speculative connection would still be behind the wall.

I do get that we have concerns about the impact to our users. Sounds like we don't have tests to gain enough confidence? If that's the case, maybe we shouldn't change it.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sean, basically you should be able to set the value to any value when specifying it as a custom preference for geckodriver. And as such it should not be overridden by the default value in geckodriver. Can you please check that this works with browsertime or whatever you make use of?

Comment 9

[Sean Feng [:sefeng211]]

Yeah, that works and it is something we are currently doing.

The motivation of this bug was it wasn't obvious that the speculative connection was disabled, and we didn't see a reason for keeping it disabled, so we thought updating it was probably making sense.

Comment 10

[Sean Feng [:sefeng211]]

Would you agree that we should leave it as is due to its potential impact to our users, Andrew?

Comment 11

[Andrew Creskey [:acreskey]]

(In reply to Sean Feng [:sefeng] from comment #10)

Would you agree that we should leave it as is due to its potential impact to our users, Andrew?

I don't have a good understanding of how users are using Marionette and Geckodriver, outside of performance work.

For performance work, we've seen that disabling the speculative connection pool is a problem since it's preventing a key optimization from working as it does in the shipped app.

We could change browsertime to reset the network.http.speculative-parallel-limit to the current default of 6, but I think this introduces some technical debt because if we ever change the pool size (which does happen to max out currently), we then have to change this value in multiple places. Not the end of the world, but seems not great.

Comment 12

[Sean Feng [:sefeng211]]

I understand that people would use Geckodriver to do test their websites, for functionalities for instance. Might not sensitive to speculative connection though.

Whimboo's concern is we don't have a good understanding of the potential impacts, and we could learn it via shipping it, but it would be too late the impact is huge.

Comment 13

[Andrew Creskey [:acreskey]]

Nick, do you have any thoughts on this?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Lets move this bug to geckodriver given that this is the place where the pref needs to be removed.

I would suggest that I file a new bug for Marionette, and get the preference removed there first. Lets see if that causes some unexpected behaviors. Then we can decide about the geckodriver case.

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #11)

We could change browsertime to reset the network.http.speculative-parallel-limit to the current default of 6, but I think this introduces some technical debt because if we ever change the pool size (which does happen to max out currently), we then have to change this value in multiple places. Not the end of the world, but seems not great.

I assume that it will always be possible to override this preference when using browsertime? I just want to make sure that if we remove it from geckodriver users of browsertime would still be able to set it back to 0 in case we hit some unforeseen behavioral changes.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #13)

Nick, do you have any thoughts on this?

I have no strong opinion. My gut instinct is that changing the speculative connection will be unobservable outside of performance (by design, right?) and that we shouldn't be shy about changing the Marionette/geckodriver defaults as Mozilla's needs change. For downstream consumers, each new Firefox/geckodriver is functionally a new product to test against; it's not like anybody can keep meaningful historical data across browser revisions. One more setting change (in geckodriver, but still) doesn't change that to my eye.

Comment 16

[Matthew N. [:MattN]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

I wasn't around when the changes for bug 992611 have been landed. As such I don't know why speculative connections have been turned off. Maybe Joel or Matthew still remember?

The bug was about memory leaks getting reported due to the speculative connection staying around so it was only a problem for harnesses that are doing leak checking. It definitely makes sense to use the default value for tests not affected by the leak. It's also possible that we can change the leak checking code to ignore this leak since it's intentional to keep the connection open so that it can be used later. Or maybe the speculative connection code should be listening for shutdown to know that speculative connections should be killed (and therefore not reported as leaks).

Comment 17

[Andrew Creskey [:acreskey]]

I assume that it will always be possible to override this preference when using browsertime? I just want to make sure that if we remove it from geckodriver users of browsertime would still be able to set it back to 0 in case we hit some unforeseen behavioral changes.

Yes, it's quite easy to override this preference in browsertime (as we are doing now to restore it to it's default value).
Actually, the master branch of the sitespeed.io browsertime repo is setting it to 0:
https://github.com/sitespeedio/browsertime/blob/52121ec7af3583987ba1ec58ba8796e49762b7c6/lib/firefox/webdriver/firefoxPreferences.js#L158

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hah, looks like they also copied the preferences 1:1 from ours. :) But at least that sounds good. So we won't cause regressions in Browsertime.

I uploaded a patch for review on bug 1617869 for Marionette. Lets wait once that landed, and then I can create a patch for geckodriver. It would then be good if you can test that before we have the 0.27 release.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sean, nothing seem to have been regressed with the landing of bug 1617869. Would you be interested to provide the fix for geckodriver?

Comment 20

[Sean Feng [:sefeng211]]

sure, I'll do a patch for it.

Comment 21

[Sean Feng [:sefeng211]]

Attachment: Bug 1613507 - [geckodriver] Don't set `speculative-parallel-limit` to `0` but leave it on its default value. r=whimboo

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5caf69708b98
[geckodriver] Don't set `speculative-parallel-limit` to `0` but leave it on its default value. r=whimboo,marionette-reviewers

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5caf69708b98

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Note that there is no new geckodriver release necessary. Starting with the Firefox Nightly build today the changes will be in effect.