Closed Bug 967693 Opened 8 years ago Closed 8 years ago

First trial of HTTP cache v2 on Desktop Firefox Nightly

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [talos_regression])

Attachments

(2 files, 5 obsolete files)

No description provided.
Attached patch v1 (obsolete) — Splinter Review
- 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
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8370371 - Flags: review?(jduell.mcbugs)
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.
Attachment #8370371 - Flags: review?(jduell.mcbugs) → review+
Depends on: 964898
(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.
Attached patch v1.1 [as landed] (obsolete) — Splinter Review
https://hg.mozilla.org/integration/mozilla-inbound/rev/f401b820cd6a

- GetCacheDir
- slightly modified way to get cache v1 dir
Attachment #8370371 - Attachment is obsolete: true
Attachment #8370932 - Flags: checkin+
Whiteboard: [KEEP OPEN]
Attached patch alt pref, v1 (obsolete) — Splinter Review
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.
Attachment #8371053 - Flags: review?(jduell.mcbugs)
Attachment #8371053 - Flags: review?(jduell.mcbugs) → review+
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/
Whiteboard: [KEEP OPEN]
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.
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
(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!
deps on landing the disable-test patch for that bug.
Depends on: 962949
One simple-to-fix crash I think worth to fix before the test.
Depends on: 950164
Attached patch v2 (obsolete) — Splinter Review
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
Attachment #8370932 - Attachment is obsolete: true
Attachment #8371053 - Attachment is obsolete: true
Attachment #8372696 - Flags: review?(roc)
Attachment #8372696 - Flags: review?(jduell.mcbugs)
Attachment #8372696 - Flags: review?(ehsan)
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.
Attachment #8372696 - Flags: review?(ehsan) → review?(gavin.sharp)
Comment on attachment 8372696 [details] [diff] [review]
v2

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

I can't review these devtools changes.
Attachment #8372696 - Flags: review?(roc) → review?(dcamp)
Attachment #8372696 - Flags: review?(dcamp) → review+
Gavin, Jason, ping on review.  We want to do this ASAP.
(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.
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?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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).
(In reply to Honza Bambas (:mayhemer) from comment #20)
> devtools are a bug unknown 

BIG unknown...
Attachment #8372696 - Flags: review?(jduell.mcbugs) → review+
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 on attachment 8372696 [details] [diff] [review]
v2

r=me to land this for not more than a few days.
Attachment #8372696 - Flags: review?(gavin.sharp) → review+
https://tbpl.mozilla.org/?tree=Try&rev=d477e4eb9cc0

Another crappy test failing, what a fun!
Attached patch additional test disable... (obsolete) — Splinter Review
Attachment #8376218 - Flags: review?(ehsan)
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.
Attachment #8376218 - Flags: review?(ehsan) → review+
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.
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?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
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
Attachment #8372696 - Attachment is obsolete: true
Attachment #8376218 - Attachment is obsolete: true
Attachment #8377350 - Flags: review?(ehsan)
Attachment #8377350 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/9635476adfeb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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?
Whiteboard: [talos_regression]
(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.
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.
(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 on attachment 8378359 [details] [diff] [review]
add the preference to talos (1.0)

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

Thanks!
Attachment #8378359 - Flags: review?(honzab.moz) → review+
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.
Depends on: 975367
No longer blocks: cache2enable
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.
Flags: needinfo?(honzab.moz)
I've added the bug to see-also field.  This is doesn't actually block that bug.  It's just related.
Flags: needinfo?(honzab.moz)
See Also: → cache2enable
Blocks: 975829
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.
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?
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.
Duplicate of this bug: 968602
Summary: Temporarily turn on HTTP cache v2 on Desktop Firefox Nightly → First trial of HTTP cache v2 on Desktop Firefox Nightly
Depends on: 1053517
You need to log in before you can comment on or make changes to this bug.