Last Comment Bug 770911 - Remove all prefetch clearing code
: Remove all prefetch clearing code
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 7
-- normal (vote)
: mozilla17
Assigned To: Brian R. Bondy [:bbondy]
: Jason Smith [:jsmith]
: Jim Mathies [:jimm]
Depends on: 770883 770899 774444
  Show dependency treegraph
Reported: 2012-07-04 08:39 PDT by Brian R. Bondy [:bbondy]
Modified: 2012-08-07 02:11 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1. (24.39 KB, patch)
2012-07-04 09:04 PDT, Brian R. Bondy [:bbondy]
taras.mozilla: review+
Details | Diff | Splinter Review
Patch v2. (25.17 KB, patch)
2012-07-31 16:09 PDT, Brian R. Bondy [:bbondy]
taras.mozilla: review+
Details | Diff | Splinter Review
Patch v3. (25.35 KB, patch)
2012-08-01 13:56 PDT, Brian R. Bondy [:bbondy]
netzen: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Brian R. Bondy [:bbondy] 2012-07-04 08:39:36 PDT
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.
Comment 1 User image Brian R. Bondy [:bbondy] 2012-07-04 08:41:45 PDT
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).
Comment 2 User image Brian R. Bondy [:bbondy] 2012-07-04 09:04:11 PDT
Created attachment 639118 [details] [diff] [review]
Patch v1.

This won't land until v17.
Comment 3 User image (dormant account) 2012-07-05 11:20:44 PDT
Comment on attachment 639118 [details] [diff] [review]
Patch v1.

   if (gotCounters && !ioCounters.ReadOperationCount)

This should also be removed.
Comment 4 User image Brian R. Bondy [:bbondy] 2012-07-05 11:26:08 PDT
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 User image Alex Keybl [:akeybl] 2012-07-13 16:58:47 PDT
Not yet tracking for 16, since bug 770899 hasn't landed yet.
Comment 6 User image Brian R. Bondy [:bbondy] 2012-07-13 19:19:22 PDT
I think I marked the wrong bug with that flag, sorry about that. Ignore.
Comment 7 User image Brian R. Bondy [:bbondy] 2012-07-16 11:53:42 PDT
The landing of this patch is on hold until we gather more data relating to the benefits of clearing prefetch on cold startups.
Comment 8 User image Brian R. Bondy [:bbondy] 2012-07-25 07:22:25 PDT
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.
Comment 9 User image (dormant account) 2012-07-30 10:04:05 PDT
(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
Comment 10 User image Brian R. Bondy [:bbondy] 2012-07-31 12:03:24 PDT
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.
Comment 11 User image Brian R. Bondy [:bbondy] 2012-07-31 16:09:56 PDT
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.
Comment 12 User image (dormant account) 2012-07-31 19:30:19 PDT
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
Comment 13 User image Brian R. Bondy [:bbondy] 2012-08-01 13:56:18 PDT
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.
Comment 15 User image Brian R. Bondy [:bbondy] 2012-08-02 07:57:18 PDT
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
Comment 16 User image Ryan VanderMeulen [:RyanVM] 2012-08-02 19:09:16 PDT
Comment 17 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-08-03 12:02:31 PDT
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.
Comment 18 User image Brian R. Bondy [:bbondy] 2012-08-04 15:09:38 PDT
Comment 19 User image Jason Smith [:jsmith] 2012-08-05 07:46:19 PDT
Verified on Win 7 using some basic sanity 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)

Note You need to log in before you can comment on or make changes to this bug.