Remove all prefetch clearing code

VERIFIED FIXED in Firefox 16

Status

()

Core
Widget: Win32
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Trunk
mozilla17
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 unaffected, firefox16 verified, firefox17 verified, firefox-esr10 unaffected)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Once Bug 770899 is on Nightly (v16) and naturally goes through a migration to Nightly v17, + 3 weeks, we should land this patch which will remove all prefetch clearing code completely.
(Assignee)

Comment 1

5 years ago
After the 3 weeks into the next cycle we'll also request that this goes onto Aurora v16.  That way code that deletes read-only .pf files will never reach beta (as it shouldn't).
(Assignee)

Updated

5 years ago
tracking-firefox16: --- → ?
(Assignee)

Updated

5 years ago
Summary: Remove all prefetch clearing code and telemetry data → Remove all prefetch clearing code
(Assignee)

Comment 2

5 years ago
Created attachment 639118 [details] [diff] [review]
Patch v1.

This won't land until v17.
Attachment #639118 - Flags: review?(taras.mozilla)

Comment 3

5 years ago
Comment on attachment 639118 [details] [diff] [review]
Patch v1.

   if (gotCounters && !ioCounters.ReadOperationCount)
 #endif
   {
       XPCOMGlueEnablePreload();
   }

This should also be removed.
Attachment #639118 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 4

5 years ago
From the b2g file? Yup I guess that was forgotten in the other bug for that functionality, but I'll include it before landing this one.

Comment 5

5 years ago
Not yet tracking for 16, since bug 770899 hasn't landed yet.
(Assignee)

Comment 6

5 years ago
I think I marked the wrong bug with that flag, sorry about that. Ignore.
tracking-firefox16: ? → ---
(Assignee)

Comment 7

5 years ago
The landing of this patch is on hold until we gather more data relating to the benefits of clearing prefetch on cold startups.
(Assignee)

Comment 8

5 years ago
Comment on attachment 639118 [details] [diff] [review]
Patch v1.

Taras any update on the prefetch priming analysis?  I'd like to land this on m-c soon.
Attachment #639118 - Flags: feedback?(taras.mozilla)

Updated

5 years ago
Depends on: 774444

Comment 9

5 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Comment on attachment 639118 [details] [diff] [review]
> Patch v1.
> 
> Taras any update on the prefetch priming analysis?  I'd like to land this on
> m-c soon.

Jason posted the analysis in bug 774444, looks like cold prefetch = better

Updated

5 years ago
Attachment #639118 - Flags: feedback?(taras.mozilla)
(Assignee)

Comment 10

5 years ago
So as mentioned on IRC I'm going to land this patch which removes the prefetch code even after the analysis in bug 774444.  I won't have spare cycles in the near future to work on this.  And if we implement some kind of prefetch priming task in the future, we can land the related code from the original prefetch code again from a clean slate.

You mentioned that prefetch gets regenerated after each update. 

I'm not entirely convinced that it's worth optimizing post-next-reboot after an update.  Most people won't reboot for 1-2 weeks after they update, so they will not see any benefit until after those 1-2 weeks.  Each new update after this, they'll have the same slower performance until they reboot again as well.

If we do want to effect the way Windows calculates the prefetch, it might be worth using the FILE_FLAG_WRITE_THROUGH flag when writing files from the updater.  This will bypass the cache completely so that the files will not be in cache on the next startup, and prefetch files will be generated in a better than normal way.  It wouldn't be as good as cold startup prefetch files, but it might be close enough to be better than 1-2 weeks of no prefetch optimizations.

Also I'm not 100% convinced that the data seen in bug 774444 will hold true in general as well. I'm afraid we'd have to implement something again before we could tell whether it's worth it or not across all computers reporting to telemetry.
(Assignee)

Comment 11

5 years ago
Created attachment 647741 [details] [diff] [review]
Patch v2.

This was previously r+. I just wanted you to do a quick pass on the nsBrowserApp.cpp files. It always enables the preload now.
Attachment #639118 - Attachment is obsolete: true
Attachment #647741 - Flags: review?(taras.mozilla)

Comment 12

5 years ago
Comment on attachment 647741 [details] [diff] [review]
Patch v2.

   // GetProcessIoCounters().ReadOperationCount seems to have little to
   // do with actual read operations. It reports 0 or 1 at this stage
   // in the program. Luckily 1 coincides with when prefetch is

get rid of this comment

+  XPCOMGlueEnablePreload(); 
add a comment that we do this because of data in bug 771745

Kinda sucks that there are two nearly identical nsBrowserApp.cpp files

I'll file a followup bug to get rid of GetProcessIoCounters-related stuff, we don't need it anymore
Attachment #647741 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 648067 [details] [diff] [review]
Patch v3.

Implemented nits. Carried forward r+.
Pushing to oak for testing and then I'll land on m-i.
Attachment #647741 - Attachment is obsolete: true
Attachment #648067 - Flags: review+
(Assignee)

Comment 14

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/242c7b638980
(Assignee)

Comment 15

5 years ago
Comment on attachment 648067 [details] [diff] [review]
Patch v3.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 770899 + other prefetch bugs
User impact if declined: We don't want the workaround from Bug 770899 that needed to land on Aurora for a couple weeks to go onto beta.  It deleted people's read only prefetch files, but no beta/release users have prefetch files in that state.  This is the last piece of code that needs to land relating to reverting the prefetch state to its old state.
Testing completed (on m-c, etc.): Testing completed on oak which is a clone of m-c.
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: none
Attachment #648067 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/242c7b638980
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [qa+]

Updated

5 years ago
QA Contact: jsmith
Comment on attachment 648067 [details] [diff] [review]
Patch v3.

looks good, approving for aurora and let's get the bug 770899 workaround backed out before merge.
Attachment #648067 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/c4f3769869bb
status-firefox15: --- → unaffected
status-firefox16: --- → fixed
status-firefox17: --- → fixed
(Assignee)

Updated

5 years ago
status-firefox-esr10: --- → unaffected
Verified on Win 7 using some basic sanity tests:

Tests

- Run latest firefox version, ensure prefetch does not turn on after 3 minutes (pass on nightly, aurora)
- Update to latest firefox version, no prefetch service in install logs (pass on nightly/aurora)
- Check telemetry dashboard to see startup_using_preload isn't showing data for latest builds (pass on latest nightly/aurora)
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
status-firefox17: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.