967693 - First trial of HTTP cache v2 on Desktop Firefox Nightly

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- pref set to on in browser/app/profile/firefox.js only
- one perma-failing test disabled (bug 948566)

There are two other intermittent oranges tracked, one of them is only a suspicion (unconfirmed) and other is a known failure *probably* happening more often with cache2.  I may be just too cautious here..

https://tbpl.mozilla.org/?tree=Try&rev=4de543b031f0

Comment 2

[Jason Duell]

Comment on attachment 8370371 [details] [diff] [review]
v1

Review of attachment 8370371 [details] [diff] [review]:
-----------------------------------------------------------------

Once we have bug 964898 this is ready to land.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #2)
> Once we have bug 964898 this is ready to land.

We can land that bug afterwords.  Or any time.  It will delete inactive cache version stuff whenever we want.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [as landed]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f401b820cd6a

- GetCacheDir
- slightly modified way to get cache v1 dir

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: alt pref, v1

I've realized we would loose early adopters after a switchback.  The v1.1 patch will effectively remove the user pref to use the new cache.  This is a prevention to keep the testers with us - a temp pref.

Comment 6

[Jason Duell]

Comment on attachment 8371053 [details] [diff] [review]
alt pref, v1

Review of attachment 8371053 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheObserver.cpp
@@ +25,5 @@
>  
>  static uint32_t const kDefaultUseNewCache = 0; // Don't use the new cache by default
>  uint32_t CacheObserver::sUseNewCache = kDefaultUseNewCache;
> +
> +static bool sUseNewCacheTemp = false; // Temp trigger to not loose early adopters

s/loose/lose/

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8371053 [details] [diff] [review]
alt pref, v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/285204b60e94

Thanks for quick review Jason!

Comment 8

[Phil Ringnalda (:philor)]

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/f401b820cd6a and https://hg.mozilla.org/integration/mozilla-inbound/rev/285204b60e94 in https://hg.mozilla.org/integration/mozilla-inbound/rev/b04e2524e2eb to investigate both whether this caused the sudden enormous increase in instances of bug 962949 and also whether it caused quite a lot of as yet unfiled failures, among which things like https://tbpl.mozilla.org/php/getParsedLog.php?id=34192079&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=34188474&tree=Mozilla-Inbound seem quite likely, and https://tbpl.mozilla.org/php/getParsedLog.php?id=34186565&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=34185065&tree=Mozilla-Inbound seem entirely possible.

Comment 9

[Honza Bambas (:mayhemer)]

Phil, thanks for the log references.  

I had suspicion around bug 962949, now confirmed.  Debugging tools code and tests are a black box for me - cache relation is unclear, however we even have one perma orange (was disabled in one of the commits).  Others I've hit few times, but later I haven't seen them on projects/gum where cache2 is constantly being ran and maintained.  When an orange was tracked, usually fixes where 50:50 in the cache and in bad tests.  Last time I more hit just bad tests.  So I really have a taste to say "Fix your badly written tests!".

I'll look at all one by one, if too complicated, I'll talk to modules owners about temp disabling some of the (mainly dbgtools) tests.

Comment 10

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

this bug has a performance win for the talos ts_paint test.  when backed out, we regressed back to the state prior to the original commit:
http://graphs.mozilla.org/graph.html#tests=[[83,131,25]]&sel=none&displayrange=7&datatype=running

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Joel Maher (:jmaher) from comment #10)
> this bug has a performance win for the talos ts_paint test.  when backed
> out, we regressed back to the state prior to the original commit:
> http://graphs.mozilla.org/graph.html#tests=[[83,131,
> 25]]&sel=none&displayrange=7&datatype=running

Yep, there are some optimizations made, more to come later.  Good news!

Comment 12

[Honza Bambas (:mayhemer)]

deps on landing the disable-test patch for that bug.

Comment 13

[Honza Bambas (:mayhemer)]

One simple-to-fix crash I think worth to fix before the test.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2

Jason, the /netwerk and pref.js code changes are identical to the last patch
Roc, please approve the /browser/devtools test disables
Ehsan, please check the remaining browser test disables (/browser modulo /browser/devtools)

Thanks.

https://tbpl.mozilla.org/?tree=Try&rev=69037952164f

Comment 15

[(no longer active)]

Comment on attachment 8372696 [details] [diff] [review]
v2

Review of attachment 8372696 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I feel comfortable reviewing this since I don't understand what the implications of disabling these tests are.  Over to Gavin.

But please use this pattern for disabling the tests:
skip-if = true # The reason goes here.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8372696 [details] [diff] [review]
v2

Review of attachment 8372696 [details] [diff] [review]:
-----------------------------------------------------------------

I can't review these devtools changes.

Comment 17

[Honza Bambas (:mayhemer)]

Gavin, Jason, ping on review.  We want to do this ASAP.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailacopolypse) from comment #15)
> But please use this pattern for disabling the tests:
> skip-if = true # The reason goes here.

I personally think the skip-if = "Explanation" is better.  When you run the tests, you can see in the log why those are actually skipped.  But I will change it.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I mentioned this on IRC: I don't understand why we want to land this with stuff broken and tests disabled, rather than focusing on actually getting it landable on trunk "for real".

Do you need help fixing the test failures?

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #19)
> I mentioned this on IRC: I don't understand why we want to land this with
> stuff broken and tests disabled, rather than focusing on actually getting it
> landable on trunk "for real".

This is meant as a temporary trial.  Private mail sent on this.

> 
> Do you need help fixing the test failures?

Definitely for the devtools failures, devtools are a bug unknown to me and relation is totally unclear to me.

Others are under control and we know how to fix them, but those fixes depends on larger work to be done first.  But I would like to do this trial before that large work gets landed - to make sure what we have now is stable (means doesn't crash and pages load).

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> devtools are a bug unknown 

BIG unknown...

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8372696 [details] [diff] [review]
v2

Review of attachment 8372696 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/test/browser.ini
@@ +74,5 @@
>  [browser_pageStyle.js]
>  [browser_privatetabs.js]
>  [browser_scrollPositions.js]
>  [browser_sessionHistory.js]
> +skip-if = "Bug 968895, Additional intermittent failures with HTTP cache v2 on"

Already identified as not related to the new HTTP cache v2.  This skip-if will be removed before landing this patch.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8372696 [details] [diff] [review]
v2

r=me to land this for not more than a few days.

Comment 24

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=d477e4eb9cc0

Another crappy test failing, what a fun!

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: additional test disable...

Comment 26

[(no longer active)]

Comment on attachment 8376218 [details] [diff] [review]
additional test disable...

Review of attachment 8376218 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if this is landing for just a few days.

Comment 27

[Honza Bambas (:mayhemer)]

Thanks everyone for their r+'s!

Now it's Friday, so I think it's better to let this for Monday (I usually don't work on weekends and there is also less users).  So I'll wait if there is no strong opinion to do this trial right now.

Comment 28

[(no longer active)]

If the goal here is to just turn this on for our Nightly *users* not the test infra, a much better way I think is to change the pref in all.js (or firefox.js or the equivalent) to turn on the v2 cache and set the pref for our tests to false.  That will basically make it unnecessary to disable these tests temporarily and will still give you nightly usage.  testing/profiles/prefs_general.js controls what prefs are used for our test infra.

Would that help you here Honza?

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #28)
> If the goal here is to just turn this on for our Nightly *users* not the
> test infra, a much better way I think is to change the pref in all.js (or
> firefox.js or the equivalent) to turn on the v2 cache and set the pref for
> our tests to false.  That will basically make it unnecessary to disable
> these tests temporarily and will still give you nightly usage. 
> testing/profiles/prefs_general.js controls what prefs are used for our test
> infra.
> 
> Would that help you here Honza?

That sounds interesting!  Thanks Ehsah!  I will create a new patch according this.

Comment 30

[Honza Bambas (:mayhemer)]

Attachment: v3 - simple and no test disabled!

As suggested by Ehsan, and works!  The bc runs would otherwise fail because of all those failing (one perma) devtools tests:

https://tbpl.mozilla.org/?tree=Try&rev=860969a23bab

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8377350 [details] [diff] [review]
v3 - simple and no test disabled!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9635476adfeb

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9635476adfeb

Comment 33

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

this is causing a lot of talos regressions, reading back this was only supposed to be for nightly users and not our test machines.  Do we want to add this pref to talos:
user_pref("browser.cache.use_new_backend_temp", false);

If not, are we going to fix this?

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to Joel Maher (:jmaher) from comment #33)
> this is causing a lot of talos regressions, reading back this was only
> supposed to be for nightly users and not our test machines.  Do we want to
> add this pref to talos:
> user_pref("browser.cache.use_new_backend_temp", false);
> 
> If not, are we going to fix this?


How bad is it?  What talos tests are regressing?  Where can I see the results?

Last time I heard about talos improvements, tho:


Comment 10: this bug has a performance win for the talos ts_paint test.  when backed out, we regressed back to the state prior to the original commit:
http://graphs.mozilla.org/graph.html#tests=[[83,131,25]]&sel=none&displayrange=7&datatype=running


Anyway, I'm not against disabling for talos as well.

Comment 35

[(no longer active)]

I think we should turn this off for Talos, and be prepared to fix these rtegressions before turning on the HTTP cache backend.  You can see the regressions in the archives of the dev-tree-management mailing list.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #35)
> I think we should turn this off for Talos, and be prepared to fix these
> rtegressions before turning on the HTTP cache backend.  You can see the
> regressions in the archives of the dev-tree-management mailing list.

How to do it?  Where is the magic file for this?

Comment 37

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

Attachment: add the preference to talos (1.0)

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 8378359 [details] [diff] [review]
add the preference to talos (1.0)

Review of attachment 8378359 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 39

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

talos bits are landed:
http://hg.mozilla.org/build/talos/rev/79c4a50dd558

will deploy tomorrow to inbound- have other changes waiting on try server results.

Comment 40

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I think we should marked this bug as blocker of main tracking cache v2 bug #913806 or should we block other bug for this with this one? It's really helpful when searching dependencies etc. for Nightly testers.

Comment 41

[Honza Bambas (:mayhemer)]

I've added the bug to see-also field.  This is doesn't actually block that bug.  It's just related.

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

I've seen a few oddities in the media mochitests in some try pushes recently.

Specifically this push:

https://tbpl.mozilla.org/?tree=Try&rev=c8e87704285d

My push has some large scale changes to the media stack in there, but this last push after I updated was significantly more orange than the previous push. In fact, I thought I'd fixed all the orange in that showed up in the previous push, so I was very surprised that the later push had some much new orange.

For example, in this mtest-1 run:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35130855&tree=Try&full=1#error2

We see a failure in test_seekable1.html, preceeded by warnings:
21:18:35     INFO -  [Parent 4004] WARNING: Can't seek backwards, so seeking to 0: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\content\media\MediaCache.cpp, line 1171
Normally this would happen when the server doesn't support HTTP1.1 byte range requests (or when we request specifically that byte range requests are disabled), so this is odd.

Also, bug 975928, when retrieving a resource via XHR, sometimes I'd get a 206 response with a fully cached resource. But I'd not specified a range request, so it's weird that one was being peformed. This also only happens when I'm running the full media mochitests (content/media/test/) rather than looping on the test in which this occurs specifically. I'd guess that media element in a previous test was loading the same resource as I request with XHR, but with a 0- range, and the necko cache is returning that request. This is kinda confusing.

Comment 43

[Honza Bambas (:mayhemer)]

Chris, not sure you what you are concerned about here.  The new cache is not enable on test infra at all.  The code has not changed for few weeks (we only develop on going larger changes on our project tree or locally).

Didn't you want to put this comment to a different bug?

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sorry, I didn't realize the new cache was disabled on the test infra. I was seeing a spike in failures on Tryserver which I couldn't explain with my own code changes and which had network related assertion failures. My bad.