Closed Bug 770911 Opened 8 years ago Closed 8 years ago

Remove all prefetch clearing code

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox15 --- unaffected
firefox16 --- verified
firefox17 --- verified
firefox-esr10 --- unaffected

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [qa!])

Attachments

(1 file, 2 obsolete files)

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.
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).
Summary: Remove all prefetch clearing code and telemetry data → Remove all prefetch clearing code
Attached patch Patch v1. (obsolete) — Splinter Review
This won't land until v17.
Attachment #639118 - Flags: review?(taras.mozilla)
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+
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.
Not yet tracking for 16, since bug 770899 hasn't landed yet.
I think I marked the wrong bug with that flag, sorry about that. Ignore.
The landing of this patch is on hold until we gather more data relating to the benefits of clearing prefetch on cold startups.
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)
Depends on: 774444
(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
Attachment #639118 - Flags: feedback?(taras.mozilla)
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.
Attached patch Patch v2. (obsolete) — Splinter Review
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 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+
Attached patch Patch v3.Splinter Review
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa+]
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+
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
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.