770911 - Remove all prefetch clearing code

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Brian R. Bondy [:bbondy]]

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

[Brian R. Bondy [:bbondy]]

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

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This won't land until v17.

Comment 3

[(dormant account)]

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

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

This should also be removed.

Comment 4

[Brian R. Bondy [:bbondy]]

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

[Alex Keybl [:akeybl]]

Not yet tracking for 16, since bug 770899 hasn't landed yet.

Comment 6

[Brian R. Bondy [:bbondy]]

I think I marked the wrong bug with that flag, sorry about that. Ignore.

Comment 7

[Brian R. Bondy [:bbondy]]

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

[Brian R. Bondy [:bbondy]]

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

[(dormant account)]

(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

[Brian R. Bondy [:bbondy]]

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

[Brian R. Bondy [:bbondy]]

Attachment: 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

[(dormant account)]

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

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Implemented nits. Carried forward r+.
Pushing to oak for testing and then I'll land on m-i.

Comment 14

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/242c7b638980

Comment 15

[Brian R. Bondy [:bbondy]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/242c7b638980

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

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

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/c4f3769869bb

Comment 19

[Jason Smith [:jsmith]]

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)