692255 - Find a way to get rid of prefetch files on Windows for faster startup

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Brian R. Bondy [:bbondy]]

I was talking to Taras on IRC and he mentioned that we could have a quick startup speed win if we could find a way to not generate, or just nuke the files inside: 
C:\Windows\Prefetch

There is a command line for /prefetch:1 that you can use that will I think change the name of the prefetch files.  This is used sometimes if you have a program that is frequently started for 2 different reasons and you want to prefetch differently via the application shortcut for each of the 2 icons.  I'm not sure if there's something here we can use to always generate different prefetch files and nuke out the old ones.

One way to get rid of the prefetch files is to use the service on demand after startup to delete the files since it runs as an elevated process.

Comment 1

[Brian R. Bondy [:bbondy]]

I talked to a friend of mine who is a very strong file system driver developer and he mentioned that in the “Windows Internals” book (Ch. 9.15.2 Logical Prefetcher) they write that prefetch can be disabled for the whole system only. 
If there was a way to do it for specified app only, i.e. with a registry value list, then this book would have listed it.

There are 2 ways we can accomplish disabling the prefetch for our app only though:
1) We can set the ACL's on the .pf files for our filename (it is the filename followed by a hex hash of the file path by default).
2) We can lock the .pf files by opening exclusively from the service.

#1 sounds the best and wouldn't even rely on the service, we could do it if elevated even from the updater or installer nsi script.
In this case the prefetch file is created but not used.

Comment 2

[Brian R. Bondy [:bbondy]]

With the about:startup extension and cold boots on Windows XP I am not seeing any difference:

main sessionRestored firstPaint version appBuildID


With prefetch files:
343	2203	1968	9.0.1	20111220165912
562	2218	2031	9.0.1	20111220165912
343	1906	1718	9.0.1	20111220165912
----------------------------------------------
416.00	2109.00	1905.67	9.0.1	20111220165912 (average)



Without prefetch files:
438	2157	1969	9.0.1	20111220165912
422	2156	1984	9.0.1	20111220165912
391	2047	1844	9.0.1	20111220165912
----------------------------------------------
417.00	2120.00	1932.33	9.0.1	20111220165912 (average)

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Not yet tested patch to nuke the prefetch files from the service.

Not ready for a review yet as I just coded it and didn't test it yet. I tried manually running prefetch tests as per the previous comment and didn't see any gain at least on XP.

Comment 4

[(dormant account)]

This is pretty annoying to test since amount of IO varies. I recommend using xperf and seeing how much io is attributed to firefox, and how many IOPs it constitutes.
Some of the problems with prefetch
* It reads in a bunch of crap before app even runs
* Crap can be all of the fonts on the system, all of the plugins, etc...all of those often get read during session restore if websites like facebook are in there... problem is that all this happens before our code runs, so it delays UI showing(.firstPaint) hugely
* Prefetch does not use windows readahead.

For a test system I used an acer i7 laptop with a crappy 5400rpm  hd, there the differences was 1.6seconds(no prefetch) vs 2.6+ with prefetch.

Also, the optimization in
http://mxr.mozilla.org/mozilla-central/source/b2g/app/nsBrowserApp.cpp#234
is currently gated on an unreliably prefetch-detection(ie it's overly conservative), make sure that codepath is live when prefetch is off in your case. Once we have a service that nukes prefetch, this code should probably be gated on (firefoxUpdateService || !ReadOperationCount)

Comment 5

[(dormant account)]

Oh yeah, in session restore case, prefetch also reads in a good chunk of our disk cache for us.

Comment 6

[Brian R. Bondy [:bbondy]]

Thanks for the info Taras, just need some minor cleanup on the patch then it should be able to go to review.

Comment 7

[Brian R. Bondy [:bbondy]]

I'm not sure how the about:startup extension works exactly but I think my data in comment 2 is probably completely invalid since the startup time that is related to prefetch probably can't be measured by an extension.

Comment 8

[(dormant account)]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> I'm not sure how the about:startup extension works exactly but I think my
> data in comment 2 is probably completely invalid since the startup time that
> is related to prefetch probably can't be measured by an extension.

it is correct. It looks at process creation times and windows does prefetch after creating the process(and attributes cost correctly via xperf)

Comment 9

[(dormant account)]

It is more likely that your hardware or profile do not exhibit the prefetch differences. These are most pronounced on underperforming harddrives.

Comment 10

[Brian R. Bondy [:bbondy]]

OK thanks for the info.  The test was done on a vmware workstation install by the way, yes my hardware is very good.

Comment 11

[(dormant account)]

don't test startup on vms.

Comment 12

[Brian R. Bondy [:bbondy]]

well that sucks :)

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Tested and it's working.  The prefetch files get cleared on app shutdown. 
It does not conflict with update shutdown using the service because that exits using exit(0) directly after detecting an update should be applied.
Taras please feel free to use this patch for testing if you want to do further speed tests than what you already found out.

---

Sorry for yet another review Rob :(.  
I couldn't think of anyone else applicable to review this though.
Please feel free to forward the review to Ehsan or someone else if you think applicable.

The other post-land maintenance service bugs are higher priority and also Ehsan's background update task is higher priority than this.  
So I expect it'll sit in queue for a bit and I'm not concerned about that, I just needed a break from update tasks :)

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Fixed text in license header.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Deleting files from under %SystemRoot% seems pretty risky to me. Such a change needs more than one data point to justify it. But, it seems like a pretty cool idea.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We should make sure we are not slowing down common cases for other common cases.

From http://en.wikipedia.org/wiki/Prefetcher:
"A second myth is that the user should delete the prefetch folder contents to speed up the computer. If this is done, Windows will need to re-create all the prefetch files again, thereby slowing down Windows during boot and program starts until the prefetch files are created—unless the prefetcher is disabled.[9][10]"

From http://en.wikipedia.org/wiki/Windows_Vista_I/O_technologies#SuperFetch:
"SuperFetch is a technology that pre-loads commonly used applications into memory to reduce their load times. It is based on the "prefetcher" function in Windows XP.[9] The purpose is to improve performance in situations where running an anti-virus scan or back-up utility would result in otherwise recently-used information being paged out to disk, or disposed from in-memory caches, resulting in lengthy delays when a user comes back to their computer after a period of non-use."

I also read the prefetch data is used during restore from hibernation/sleep (at least in Windows Vista+), and that if the prefetch data is missing, our executables will be paged in from disk instead of from the optimized (already defragmented, and designed to eliminate seeking) hibernate area of the disk. So, there is a strong chance that waking up from hibernation and/or (hybrid) sleep may be slower without the prefetch data. We should verify that deleting the prefetch data doesn't adversely affect wakeup from hibernate, because there are a lot of people that always use sleep/hibernate/wake instead of powering the system off/on.

I guess that with this patch, Windows will re-create the prefetch files after we delete them, sometime while Firefox is running. I wonder how often Windows refreshes them now (when are aren't deleting them)? It seems likely there is no performance difference regarding this, but it is another thing that would be good to verify.

Also, we should measure the impact on users with SSDs, to make sure they benefit from it. If not, it would be better to avoid doing it for SSD users.

Comment 17

[Brian R. Bondy [:bbondy]]

I'm aware of the documentation you gave. This task is not driven by a blind assumption like common "delete your prefetch for faster performance" sites which are wrong.  It is driven by Taras' data and testing.  See Comment 4 for some of his analysis.

I agree that we need more results though and possibly to test things like sleep/hibernate as you mentioned before taking this patch.  I'm not sure we care as much about sleep/hibernate though vs application startup speed.

Comment 18

[(dormant account)]

(In reply to Brian R. Bondy [:bbondy] from comment #17)

> I agree that we need more results though and possibly to test things like
> sleep/hibernate as you mentioned before taking this patch.  I'm not sure we
> care as much about sleep/hibernate though vs application startup speed.

We do not. I find it a bit upsetting that someone would assume that we'd do all this work without some basic googling. Some further googling would've shown that I had a series of blog posts on this(and a ton of discussion in bug 627591).

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Taras Glek (:taras) from comment #18)
> (In reply to Brian R. Bondy [:bbondy] from comment #17)
> We do not.

We should care about about wakeup from hibernate time because that is how many people "start" Firefox.

> I find it a bit upsetting that someone would assume that we'd do
> all this work without some basic googling. Some further googling would've
> shown that I had a series of blog posts on this(and a ton of discussion in
> bug 627591).

There's no need to be upset. I just copy/pasted from Wikipedia so that my comments would make sense.

One thing that wasn't clear from your blog posts (all of which I read, BTW) is how this helps people who boot into the welcome/login screen. Will prefetch will preload Firefox while the user is still on the welcome screen. If so, we should consider how much time (if any) people spend on the welcome screen.

I was sure you had more data points but I couldn't find them. Thanks for the pointer to bug 627591 comment 56 and below.

Comment 20

[(dormant account)]

Comment on attachment 586811 [details] [diff] [review]
Patch v2.

I would prefer prefetch-deletion be automatic (controlled by a registry flag) rather than requested through IPC. Rob once wrote a windows script to monitor the prefetch dir for firefox*.pf and delete it(I can't find the bug that got attached to). 

Would be good to have an easy way to check if prefetch was nuked on startup(ie is there a cheap check to see if the updater process is around?). To replace the overly conservative prefetch check I mentioned earlier.

Comment 21

[Brian R. Bondy [:bbondy]]

> I would prefer prefetch-deletion be automatic (controlled by a registry flag)

You can do a ReadDirectoryChangesW, FindFirstChangeNotification, or you can query the NTFS journal (change log) periodically.  FindFirstChangeNotification sounds the most practical.
I'm not aware of a registry only check as you mentioned but I'll let rs chime in.

We don't keep the service running so this watching would have to happen in application code and then start the service command to delete the files upon detection.  I chose on shutdown because it has no performance difference on the firefox process while it's running.  It isn't a perfect way to always have the prefetch files deleted, but it covers most cases.

We could also do something like how the jumplist is built periodically every couple minutes with a js timer.  But on a longer timer.  Personally I prefer on shutdown only though. 

> Would be good to have an easy way to check if prefetch was nuked on startup
>(ie is there a cheap check to see if the updater process is around?)

There is an easy way to check both whether the service is available, and to check if .pf files existed during startup (i.e. if the FIREFOX-_hash_.pf file exists in %SYSTEMROOT%\prefetch.  I'll do another patch for the check you mentioned earlier.

Comment 22

[Jim Mathies [:jimm]]

We should attach this behavior to a hidden pref so people can control it. Should be pretty easy to do, just grab the pref higher up in main when the pref service is available.

Comment 23

[Brian R. Bondy [:bbondy]]

I think on shutdown + a timer could be pretty simple and low impact off the main thread.  We'd just need to check for a single file's existence and if it exists execute a service command.  The check on shutdown would remain. 

Please advise if you agree/disagree.

This reminds me it would be nice to have a shared Windows worker thread, Jim and I previously discussed it a little.

Comment 24

[(dormant account)]

(In reply to Brian R. Bondy [:bbondy] from comment #23)
> I think on shutdown + a timer could be pretty simple and low impact off the
> main thread.  We'd just need to check for a single file's existence and if
> it exists execute a service command.  The check on shutdown would remain. 

I'm a bit reluctant to findfile() in the huge prefetch directory as that might be expensive(but we can just measure that with telemetry).

You convinced me. Firefox should just monitor the prefetch dir and then execute the purging command when it notices the pf file as you suggest.

I don't think we should do anything on firefox shutdown. App shutdown may not even happen in case of crash/etc. Having a single file handle to watch the system directory seems like reasonable overhead.

> 
> This reminds me it would be nice to have a shared Windows worker thread, Jim
> and I previously discussed it a little.

Not sure what worker stuff is referring to.

Comment 25

[Brian R. Bondy [:bbondy]]

I wasn't suggesting a findfile type approach, i was suggesting a single IO which checks the direct path of where the prefetch file should be stored periodically.  It will always be the same path.

Comment 26

[(dormant account)]

(In reply to Brian R. Bondy [:bbondy] from comment #25)
> I wasn't suggesting a findfile type approach, i was suggesting a single IO
> which checks the direct path of where the prefetch file should be stored
> periodically.  It will always be the same path.

I'm not sure that there is a distinction. The prefetch filename is an unknown(due to hash), so you have to list the directory to look for it existing, right?

Comment 27

[Brian R. Bondy [:bbondy]]

I thought maybe the hash was predetermined like a CRC of the filepath maybe, and then take the hex digits of that crc, and could be calculated.

After all windows needs to have a way to tell the difference between differently named exes.

Comment 28

[Brian R. Bondy [:bbondy]]

... between the same named exes in different folders.

Comment 29

[Brian R. Bondy [:bbondy]]

This site gives info on how to calculate the prefetch file path:
http://www.woanware.co.uk/?page_id=173

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Instead of deleting the prefetch files this patch will instead set them to a 0 sized read only files.  Windows does not try to use the file nor re-create it in that case.

This saves extra IO since the prefetch files don't even attempt to get written in the first place.

This command is currently being executed on each install and on each upgrade.  But since we don't know the algorithm for the hash of the prefetch filename the prefetch file must first exist. 

Before asking for a review, I'd like to do a 1 time operation a couple minutes after the browser is started to clear the prefetch files.  This will cover the case of a new install, so a new install won't have to wait until the next upgrade to have the benefits of no prefetch file.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Jim, I was wondering if you could do the initial review on this and I'll get rstrong to super review it after?

Description of the task:

Implemented a 60 second one shot timer which starts a thread to start the service command. Starting the service command could take some time so shouldn't be done on the main thread.  

The timer and thread are only used if a preference does not exist (or is false).  After the timer gets executed the preference gets set.  So this means this only happens at most once via application code.

Each application update and install will clear the prefetch files and replace them with 0 byte files that are read only.  Windows does not write the .pf files in this case nor use them.  

You might wonder why we don't just rely on the install to clear the .pf files, that's because the service command only works if the .pf files already exist.  There is no easy way to get the filenames of the .pf files, so we do it to all FIREFOX- prefixed .pf files.

---

I will be running some tests of my own with xperf to see the effectiveness of the .pf clearing exactly, but to me it seems to start snappier :).  I have the callstacks working in xperf btw Taras.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4'. Rebased to tip

Rebased to m-c tip, not sure in what order my patch queue will land so I'm keeping both of these copies around :)

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 592846 [details] [diff] [review]
Patch v4'. Rebased to tip

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

I don't understand the changes in updatehelper.cpp/pathhash.h, so maybe rob should cross check those. Are those changes intended to be part of this patch?

All-in-all looks ok to me with nits addressed.

::: browser/components/shell/src/nsWindowsShellService.cpp
@@ +27,5 @@
>   *  Asaf Romano    <[e-mail]>
>   *  Ryan Jones     <[e-mail]>
>   *  Paul O'Shannessy <[e-mail]>
>   *  Jim Mathies    <[e-mail]>
> + *  Brian R. Bondy <[e-mail]>

Just a note - these headers are going to be purged in the near future :/ so there's no need to add these annotations anymore.

@@ +806,5 @@
>  
>    return regKey->Close();
>  }
>  
> +nsWindowsShellService::nsWindowsShellService() : mCheckedThisSession(false) 

nit - wrap this

@@ +836,5 @@
> +  if (!prefs || 
> +      NS_FAILED(prefs->GetBranch(nsnull, getter_AddRefs(prefBranch))) ||
> +      (NS_SUCCEEDED(prefBranch->GetBoolPref(kPrefetchClearedPref, 
> +                                            &prefetchCleared)) &&
> +       prefetchCleared)) {

Lets use the Preferences utils here.

@@ +846,5 @@
> +  mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +  if (mTimer) {
> +    mTimer->InitWithFuncCallback(
> +      nsWindowsShellService::LaunchPrefetchClearCommand, 
> +      nsnull, 60000, nsITimer::TYPE_ONE_SHOT);

nit - move the time value to a define at the top of the file with a nice comment explaining what it controls.

@@ +877,5 @@
> +    updaterServiceArgv[0] = L"MozillaMaintenance";
> +    updaterServiceArgv[1] = L"clear-prefetch";
> +    if (StartServiceCommand(NS_ARRAY_LENGTH(updaterServiceArgv), 
> +                            updaterServiceArgv) != ERROR_SUCCESS) {
> +      return NS_ERROR_FAILURE;

There's no reason to return a failure here, Run() ignores the return result. If it's important for a failure here to get noticed, I'd suggest an NS_WARNING() and return NS_OK.

@@ +905,5 @@
> +  nsCOMPtr<nsIPrefService> prefs =
> +    do_GetService(NS_PREFSERVICE_CONTRACTID);
> +  if (prefs) {
> +    if (NS_SUCCEEDED(prefs->GetBranch(nsnull, getter_AddRefs(prefBranch)))) {
> +      prefBranch->SetBoolPref(kPrefetchClearedPref, true);

Preferences.

Comment 34

[Brian R. Bondy [:bbondy]]

> I don't understand the changes in updatehelper.cpp/pathhash.h, so maybe 
> rob should cross check those. Are those changes intended to be part of this patch?

Yup, this is just moving around code to get things to link properly.  It allows some code reuse for starting the service from within Firefox.  Previously we could only call those functions from updater.  Rob will be able to advise on that. 

> Just a note - these headers are going to be purged in the near future :/ 
> so there's no need to add these annotations anymore.

OK cool thanks for the heads up.

> Lets use the Preferences utils here.

I don't think we can here, I did at first and got a linking error.  The rest of the file uses preference code like that. 

> There's no reason to return a failure here, Run() ignores the return result.

Ya I knew it wouldn't be used but it felt more comfortable that way.  You're right though it raises questions about if it is important that something should be logged.  I added a comment instead and returned NS_OK as you mentioned since it isn't important if this fails.

Comment 35

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5.

Implemented nits, carrying forward r+. 

This is lowest priority of the reviews by the way Rob, background updates is more important as well.

Comment 36

[Alfred Kayser]

I would move the LOG statement of the failing WritePrefetchClearedReg function inside that function, just as the other LOG statement is also there.
+  if (retCode != ERROR_SUCCESS) {
+    LOG(("Could not open key. (%d)\n", retCode));
+  }
And, add a 'return false' here after the LOG.

Comment 37

[Brian R. Bondy [:bbondy]]

I think you meant to put Comment 36 in Bug 727864?

Comment 38

[Brian R. Bondy [:bbondy]]

Note for reviewer, you can grab a build that includes this fix from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ (with -elm/ suffix)

Comment 39

[Kshitij Chawla]

(In reply to Brian R. Bondy [:bbondy] from comment #35)
> Created attachment 594717 [details] [diff] [review]
> Patch v5.
> 
> Implemented nits, carrying forward r+. 
> 
> This is lowest priority of the reviews by the way Rob, background updates is
> more important as well.

I was wondering if the priority should be bumped up. Slow Firefox cold startup is one of the chief complaint I hear from users as the reason for preferring alternates, usually chrome. A fix landing in 13 would be helpful in convincing more users to return/switch to Fx.

Comment 40

[Jim Mathies [:jimm]]

As much as we'd like to see the speedups from work like this, mucking with system files / services warrants a lot of bake time, especially since we switched to rapid release.

Comment 41

[Robert Strong (they/them - no direct email)]

Comment on attachment 594717 [details] [diff] [review]
Patch v5.

This is toolkit code and you are adding Firefox specific functionality. Instead of making it Firefox specific, I'd suggest either including the value(s) needed (e.g. FIREFOX.EXE-*.pf) in a file in the install directory or the registry.

Comment 42

[Brian R. Bondy [:bbondy]]

OK that sounds like a good idea.

Comment 43

[Taylor Cheetham]

I am worried that this could actually hurt performance in later windows versions, namely windows 8. Windows 8 now uses all available free ram as cache (as far as i am aware win7 only used some)until otherwise needed, i have been testing windows 8 for awhile now and ff opening even from a restored session has been near instant. If i didnt know better i would even go so far as to say it had been minimized, however i am not sure what windows 8 uses to determine what should and should not be in this cache. 

I assume it uses the prefetch and superfetch and if this is the case would not this change actually hurt performance in later OS's? i understand that windows 8 is only in the preview stage and that even once its out adoption will be slow but this is something to consider. Perhaps adding something that trys to detect what version of windows you have before changing anything? and if 8 is found then leave it alone. 

Although this all hinges on if windows 8 is using the super/prefetch for determining what to throw into ram like i assume it is. So far i have been testing the consumer preview since it was available using it as my main os (in a dual boot just in case) to try and put it through its paces and so far its showing to be very intelligent with its caching and i just want to make sure this change wont break that.

Comment 44

[Brian R. Bondy [:bbondy]]

I agree and I think it would be a good idea to only enable the optimization for OS that we have determined and tested to have a faster startup with no prefetch.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Changes after superreview. Patch v1.

Sorry for taking so long to get back to this.
So the changes are to make it easy to work with all apps, not just Firefox.
I store the filename to clear inside the hashed install subkey of the maintenance service registry key.  

Example: HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService\a4948786b022cc1a376991b44efb838c with string value name prefetchProcessName, and value FIREFOX

If you want to apply the patches to try it out:
1. Apply Attachment 594717 [details] [diff]: Patch v5.
2. Apply the patch from bug 727864
3. Apply this new patch (Changes after superreview. Patch v1.)

Comment 46

[Taylor Cheetham]

Alright so i messed around with this, and it would appear that as far as i can tell unless something else is messing it up that this has indeed slowed firefox loading win8. Now mind you its still fairly fast but not as fast as before the patch and unlike before i notice HD activity now so i think that win8 does indeed use pre/superfetch for figuring out what should go into its ram cache, i will be giving it a few more days to figure out if the cache can figure itself out and somehow readd firefox

Comment 47

[ferongr]

Did some testing with about:startup loading my normal profile in the latest Nightly build. System is Windows XP SP3 without hardware acceleration enabled. HDD is a mid-performance WD Blue series 240GB.

The first set of measurements are before nuking the prefetch files. Nightly was started from a shtortcut loading a Gmail AppTab and about:startup itself. Before shutting down the browser I let Gmail finish loading. Before restarting the browser I let the system idle for 5-10 seconds.

The second set of measurements was taken after nuking the Firefox prefetch file in "%windir%\Prefetch".

Note the order is newest first.

main | firstPaint | sessionRestored |createTopLevelWindow |firstLoadURI |startupCrashDetectionBegin | startupCrashDetectionEnd

Prefetch:

93	3297	3484	593	3484	172	undefined
94	3312	3500	594	3484	172	undefined
93	3312	3500	593	3484	172	undefined
94	3344	3516	594	3516	172	undefined
110	3391	3579	625	3579	188	undefined
93	3312	3500	578	3484	171	undefined
93	3312	3500	593	3484	156	undefined
94	3313	3500	594	3485	172	undefined
94	3297	3485	578	3469	156	undefined
140	3375	3546	656	3546	218	undefined

After deleting prefetch file:

125	3407	3594	688	3579	204	undefined
141	3406	3594	688	3578	219	undefined
141	3406	3594	687	3578	203	undefined
141	3406	3531	703	3531	219	undefined
141	3454	3641	704	3625	219	undefined
125	3390	3578	672	3562	203	undefined
125	3407	3594	657	3594	203	undefined
125	3407	3594	672	3579	204	undefined
156	3453	3641	719	3625	234	undefined
500	3750	3922	1016	3922	578	undefined

Comment 48

[(dormant account)]

Note, this patch is just a big step towards big startup wins. Once we delete prefetch, we need bug 727864(trivial) to ensure we do our own readahead.

Also in synthetic tests, windows prefetch is often not as polluted with irrelevant stuff(ie network cache, fonts, plugins) as it is on a real profile with multiple tabs open, etc.

We'll evaluate IRL startup impact via telemetry.

Comment 49

[Manoj]

Nominating for [snappy].

Comment 50

[Nathan Froyd [:froydnj]]

(In reply to Manoj from comment #49)
> Nominating for [snappy].

Please use the whiteboard for this sort of thing.  Somebody will remove the whiteboard annotation if it's not appropriate.

Comment 51

[Jason Smith [:jsmith]]

Brian - Could I get an updated try build with your patch? I could take a look on multiple windows builds to see what happens right now.

Comment 52

[Brian R. Bondy [:bbondy]]

> Brian - Could I get an updated try build with your patch? 

I pushed this along with bug 727864 to elm and started a Nightly build.  
You should be able to get the build in a few hours here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Comment 53

[Brian R. Bondy [:bbondy]]

Attachment: Patch v6.

Rebased to m-c tip and combined both patches into one.

Comment 54

[Robert Strong (they/them - no direct email)]

Comment on attachment 623703 [details] [diff] [review]
Patch v6.

I'm not sure why it wouldn't be better to just
a) clear prefetch during installation from the installer
b) clear prefetch after application update either the way it is cleared in this patch or from the helper when calling the elevated PostUpdate

This would remove the need to do this in the client altogether but perhaps I am missing / forgot something here.

Comment 55

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #54)
> Comment on attachment 623703 [details] [diff] [review]
> Patch v6.
> 
> I'm not sure why it wouldn't be better to just
> a) clear prefetch during installation from the installer
> b) clear prefetch after application update either the way it is cleared in
> this patch or from the helper when calling the elevated PostUpdate
Note that doing this in PostUpdate would also have an immediate affect since it is running the code for the new version after an update.
 
> This would remove the need to do this in the client altogether but perhaps I
> am missing / forgot something here.

Comment 56

[Brian R. Bondy [:bbondy]]

Do you mean via an NSIS plugin? Or perhaps just call into maintenanceservice.exe with a special command line on installs and in PostUpdate?  I think probably the later, and I think that would simplify this code.

Comment 57

[Robert Strong (they/them - no direct email)]

Either way though it might be cleaner to just do it all with NSIS. We already have the Access Control NSIS plugin in the repo for setting permissions.
http://nsis.sourceforge.net/AccessControl_plug-in

I think either way this will be much cleaner and lessen the code running in the client.

Comment 58

[Robert Strong (they/them - no direct email)]

btw: there might be value in doing this via the service that I haven't thought of so please take that into consideration as well.

Comment 59

[Brian R. Bondy [:bbondy]]

Ya the only difference is system account vs administrative elevated account, but I think every administrator will have access anyway.  This is also nice because it won't need the service.

Comment 60

[Brian R. Bondy [:bbondy]]

oh I remember now, it's been a while since I worked on this.

So the reason for the application code is that the Firefox PF file will not be created on the first time installs.  So we'd have to wait until after it is created and then initiate the service command.  Maybe the amount of people that will be in this situation though is not worth the extra complexity.  

That's the reason it's in the service too, so that an unelevated process can execute it.

Comment 61

[Brian R. Bondy [:bbondy]]

Another side effect of using the service is that when it lands on m-c, every channel you have installed, if you have the Nightly build installed, will have faster Firefox startup. I don't know if that's a good thing or not (in case there are regressions with the task), but just thought I'd mention it, in case the risk is undesirable.  

There is no way to distinguish which PF file is for what.

Comment 62

[Brian R. Bondy [:bbondy]]

Jason Smith have you been able to test the build I made on elm for you yet? I was wondering if everything is working as it is expected on all OS.

Comment 63

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #62)
> Jason Smith have you been able to test the build I made on elm for you yet?
> I was wondering if everything is working as it is expected on all OS.

Not yet. Clarification - Should I be using http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-elm/? (First time using an elm build)

Comment 64

[Brian R. Bondy [:bbondy]]

Attachment: Patch v7.

Added timer cancel and set to NULL in the destructor.

rstrong and I talked on the phone today.  We discussed that new installs (which includes both new users and people installing the browser on a new computer) will not have the firefox prefetch files when the installer runs.  So these users if we did an installer clear prefetch + update only wouldn't be cleared for 6 weeks until the next update.   Even 10 years from now it would be the same case.  So we decided to keep it the way it currently works.  Faster startup will happen on the second launch assuming the first launch lasts at least 1 minute.

Comment 65

[Brian R. Bondy [:bbondy]]

> Not yet. Clarification - Should I be using 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-elm/? 
> (First time using an elm build)

Yup I think you can use that or any date for an elm build that is after 2012-05-15

Comment 66

[Robert Strong (they/them - no direct email)]

Comment on attachment 624993 [details] [diff] [review]
Patch v7.

r=me as long as there is a clear path for disabling this on systems that get it.

Comment 67

[Brian R. Bondy [:bbondy]]

> r=me as long as there is a clear path for disabling this on systems that get it.

If it had to be reverted, we'd have to push out a patch that does a 1 time delete of the prefetch to get rid of the 0 byte read only prefetch files.

Comment 68

[Jason Smith [:jsmith]]

Did a very quick test with the current nightly build vs. the elm build containing this patch below. Will do something more extensive soon. Brian - Is there a listing somewhere of common startup performance metrics that have measured in the past for Desktop Firefox? Where I can find the telemetry on this?

Ran 5 times in a row with a new profile on Win 7 64-bit, data from about:startup extension

Win 7 64-bit - Elm Build with Patch (Averages of 5 consecutive runs)

Main - 79.00
startupCrashDetectionBegin - 163.00
firstPaint - 32739 (note - 4 of the other runs had "undefined" here)
sessionRestored - 1624.60
createTopLevelWindow - 1735.20
firstLoadURI - 490.20
startupCrashDetectionEnd - 766.00

Win 7 64-bit - Nightly Build without Patch (Averages of 5 consecutive runs)

Main - 81.00
startupCrashDetectionBegin - 176.00
firstPaint - 1504.00
sessionRestored - 1621.00
createTopLevelWindow - 469.20
firstLoadURI - 748.00

Comment 69

[Jason Smith [:jsmith]]

Win XP - Firefox 13 Beta (Does Not Have Patch, Averages of 5 consecutive runs)

Main - 115.40
startupCrashDetectionBegin - 218.60
startupCrashDetectionEnd - 31000.00
firstPaint - 759.20
sessionRestored - 809.20
createTopLevelWindow - 315.20
firstLoadURI - 678.00

Win XP - Elm Build with Patch (Averages of 5 consecutive runs)

Main - 109.00
startupCrashDetectionBegin - 215.60
firstPaint - 871.80
sessionRestored - 937.40
createTopLevelWindow - 428.00
firstLoadURI - 821.60

Comment 70

[Brian R. Bondy [:bbondy]]

As part of the testing by the way:

1) Please ensure the prefetch pf files are getting reset to 0 byte files on first startup after 1 minute.  You should physically check that the old firefox-somehash are 0 bytes and set to readonly.  Before you run the test you should ensure they are not already 0 bytes and not read only.  Once this one time operation is done app.update.service.prefetchCleared will be set to true and it should not be done again.

2) Please ensure that the prefetch pf files are getting reset to 0 bytes on updates.  You can download the day before build of elm and update to the current.

3) Please see a log entry after prefetch here: 
C:\ProgramData\Mozilla\logs\maintenanceservice.log (older log files will be renamed in the same directory, 10 logs are kept)
You should ensure that it doesn't say anything that looks out of the ordinary.  If you do see something let me know and I can tell you if it's normal or not.

4) Please ensure that the 0 byte pf files are not put back to non 0 bytes after using the browser for a couple days.

Comment 71

[Brian R. Bondy [:bbondy]]

This task is a system wide change by the way.  So if you are using the same computer for both tests, and you have cleared the prefetch ever on that computer, then you will be getting the benefit in both tests no matter which build you are using.

For a valid speed test you'd have to backup your pf files and restore them before running the non elm build.  Also for speed tests you'd have to make sure that for the elm build all tests had the prefetch cleared already.

Comment 72

[Jason Smith [:jsmith]]

Thanks for the insight Brian. I burned through creation of some of the test plan here - https://wiki.mozilla.org/Start-up_Performance_Improvements/TestPlan, which includes your feedback you've provided. When I finished, I'll put it out for review to get your feedback.

Also, I took a look at the Prefetch folders on two machines I've tested:

- Windows XP - 2 Firefox.EXE PF files have 0 KB, 2 other Firefox PF files are greater than 0 KB (55 KB and 89 KB respectively)
- Windows Vista - 2 Firefox PF files has 0 KB, 1 other Firefox PF file are greater than 0 KB (210 KB)

Comment 73

[Jason Smith [:jsmith]]

Another machine analyzed:

- Windows 7 64-bit: - 7 Firefox PF Files have 0 KB, 3 other Firefox PF files are greater than 0 KB (95 KB, 99 KB, 99KB)

Hmm. That's three machines. Could the implementation here be missing certain PF files to write 0 KB to? Naming schemes of these files include:

- FIREFOX SETUP 12.0.EXE-43D73445.pf
- FIREFOX-15.0A1.EN-US.WIN32.IN-344E47C3.pf
- FIREFOX.EXE-192E7DD2.pf

Brian - Any ideas?

Comment 74

[Brian R. Bondy [:bbondy]]

Attachment: Patch v8

The patch v7 that I added, only added a cancel and set to NULL for the timer, but the patch itself that I attached dropped a bunch of the files.  I attached a fixed patch v8 that includes all the files. 

I'm remarking as r? since you didn't put r+ on all the files because  a lot of them were missing from the patch v7.  Sorry for the inconvenience.

(Note for QA: Elm does include all the files by the way)

Comment 75

[Brian R. Bondy [:bbondy]]

Re Comment 73 jsmith: 
> Brian - Any ideas?

The files that should be cleared are only the ones of this format: 
FIREFOX.EXE-########.pf

These 2 should not be touched:
FIREFOX SETUP 12.0.EXE-43D73445.pf
FIREFOX-15.0A1.EN-US.WIN32.IN-344E47C3.pf
If other files are touched by the way, that would be a bug and should be reported.

It's not clear to me from your results whether you did verify that all prefetch files were not 0 bytes before testing, and what happened directly after the clear prefetch operation.  Also it is very important to check that the read only attribute is being set on each prefetch file that is 0 bytes after the clear operation.
Please also check the log, it will contain the status of each file of the form: FIREFOX.EXE-########.pf.  It will say whether it was deleted or not and if not why not.

For example maybe you started a firefox after the clear opeartion in another directory that created a new pf file for that firefox. 
Maybe in your build no pf files are being cleared and the results you see are from a previous install? 
Maybe all of them were cleared successfully and new ones were created after.

Also I want to stress the difference that this task effects the whole computer. So even if you use a Firefox release build, those pf files will be cleared as well and the startup of that installed firefox will be altered as well forever on the machine.
It's a good idea once you have all the pf files to make a backup of them so you can restore them.

You can delete the 0 byte read only files and Windows should eventually re-create the pf files, you may need a reboot after that.

Comment 76

[Jason Smith [:jsmith]]

Sounds good. Thanks for the insight. I did manage to see a case (for learning purposes) that 0 KB files did exist that were read only, although I'll keep the above comments you've stated above in mind.

Also - Filed bug 756846 (I'll track problems with this feature by marking them as dependencies against this bug).

Comment 77

[Brian R. Bondy [:bbondy]]

> Sounds good. Thanks for the insight. I did manage to see a case (for learning
> purposes) that 0 KB files did exist that were read only, although I'll keep the
> above comments you've stated above in mind.

If that is the case then please attach the log (as per Comment 70) of that operation.  And let me know the name of the file that was not cleared, because the reason will be in the log. 

> Also - Filed bug 756846 (I'll track problems with this feature by marking 
> them as dependencies against this bug).

Was the preference cleared before trying? It's a 1 time operation.  See Comment 70 for details.

Comment 78

[Brian R. Bondy [:bbondy]]

> I'll track problems with this feature by marking them as dependencies 
> against this bug

Thanks that sounds ideal

Comment 79

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #77)
> 
> > Also - Filed bug 756846 (I'll track problems with this feature by marking 
> > them as dependencies against this bug).
> 
> Was the preference cleared before trying? It's a 1 time operation.  See
> Comment 70 for details.

No it was not cleared (it was set to true).

Comment 80

[Brian R. Bondy [:bbondy]]

OK cool, so that explains that problem.

Comment 81

[Brian R. Bondy [:bbondy]]

On Jason Smith [:jsmith] 2012-05-18 16:01:52 PDT wrote:

> When I finished [the test plan], I'll put it out for review to get your feedback.

Any update on this? In particular there should be test cases that are being run with expected results.

Comment 82

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #81)
> On Jason Smith [:jsmith] 2012-05-18 16:01:52 PDT wrote:
> 
> > When I finished [the test plan], I'll put it out for review to get your feedback.
> 
> Any update on this? In particular there should be test cases that are being
> run with expected results.

I know that. I'm still working on it in here - https://wiki.mozilla.org/Start-up_Performance_Improvements/TestPlan and in MozTrap. Note - Traditionally the documentation itself doesn't have to be completed until a week or so into Aurora.

To merge from Nightly --> Aurora, the feature just needs to land, bare bones functionality needs to work to prove that the feature has landed, and no significant regressions (e.g. crashes). Note that QA work traditionally happens in the Aurora cycle (although there are exceptions, such as the web apps desktop feature). If there's disagreement on this, please let me know.

For the beta merge, I'd want a higher level of quality (smoke & feature signoff test cases to pass, no pref regressions, proof of start-up improvement, and no functional regressions). Release needs to fall in line with this as well.

Comment 83

[Brian R. Bondy [:bbondy]]

> Note that QA work traditionally happens in the Aurora cycle (although there are
> exceptions, such as the web apps desktop feature). 
> If there's disagreement on this, please let me know.

Yes there is disagreement. I think there should be test cases defined with the steps that are taken and the expected results. I think there is a lot of time being wasted on both of our parts currently on questions about what is actually being tested, how it is being tested, and if it is being tested in the right way.  And cross referencing old comments.  

If we had that we could simply say I ran the steps of test 5 and it is failing, and I would have all the info I need.  If this is something you would rather I do, I can write it up tomorrow.  Please advise.

Comment 84

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #83)
> > Note that QA work traditionally happens in the Aurora cycle (although there are
> > exceptions, such as the web apps desktop feature). 
> > If there's disagreement on this, please let me know.
> 
> Yes there is disagreement. I think there should be test cases defined with
> the steps that are taken and the expected results. I think there is a lot of
> time being wasted on both of our parts currently on questions about what is
> actually being tested, how it is being tested, and if it is being tested in
> the right way.  And cross referencing old comments.

Then I disagree. That's typical for someone ramping up on how to test something to ask questions, define testing scope, etc. I don't see a reason why there's such an urgency to have all of the testing done immediately - basic exploratory testing has already revealed that the implementation isn't causing serious regressions in firefox. The testing in the Aurora cycle in a known establishment in the firefox release process - features get tested in Aurora, beta has its own extensive testing as well. Exceptions exist, but I'm not convinced this is an exception. The test plan in that case would need to be finished near the start of the Aurora cycle and executed for the remaining pieces of the cycle. That was originally what I was planning to do.

> 
> If we had that we could simply say I ran the steps of test 5 and it is
> failing, and I would have all the info I need.  If this is something you
> would rather I do, I can write it up tomorrow.  Please advise.

I'm more than willing to drive the testing on this and have already started writing the test plan. I just need to finish writing up the perf metrics to measure and how to analyze them and writeup test cases in MozTrap. Questions will probably get asked as the test cases get built though.

Sounds like there's a large disagreement on Desktop QA process that's worth a discussion (I think Juan, our QA lead for desktop, would appreciate the feedback).

Comment 85

[Brian R. Bondy [:bbondy]]

Actually I was hoping for this feature to be tested on elm first before landing on Nightly even.

This task effects all installed products as soon as it lands and so has special needs.  Anyone that installs it will have immediate effects on Release, Beta, Aurora, Nightly, and other channels. 

The suggestion for the test plan was just a suggestion for us to work optimally.  I did not mean to tell you how to do your job but just to express the specific needs for this task.

Comment 86

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #85)
> Actually I was hoping for this feature to be tested on elm first before
> landing on Nightly even.
> 
> This task effects all installed products as soon as it lands and so has
> special needs.  Anyone that installs it will have immediate effects on
> Release, Beta, Aurora, Nightly, and other channels. 

Ah, that's important to know. Thanks for the notification.

> 
> The suggestion for the test plan was just a suggestion for us to work
> optimally.  I did not mean to tell you how to do your job but just to
> express the specific needs for this task.

Okay. The two documents I am building right now are:

- https://etherpad.mozilla.org/startup-perf-improvements-test-ideas <-- test case ideas go here
- https://wiki.mozilla.org/Start-up_Performance_Improvements/TestPlan <-- finalized test plan goes here

If you'd like to help, then feel free to contribute to the etherpad.

Comment 87

[Brian R. Bondy [:bbondy]]

We want to get this in for v15 so I plan to land this over the weekend.  Please let me know if you see any showstoppers to do that if you notice any jsmith. Thanks!

Comment 88

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #87)
> We want to get this in for v15 so I plan to land this over the weekend. 
> Please let me know if you see any showstoppers to do that if you notice any
> jsmith. Thanks!

Exploratory testing on the sanity level hasn't really caught any red flags so far. I've been running for a few days with 0 KB FIREFOX.PF prefetch file that's read only, haven't really seen anything bad happen yet. Only weird thing I saw (but this is probably out of scope) was that upon installing a nightly build and clicking finish, windows froze - putting it into sleep and restarting got nightly to finish completing the installer. Don't think that's relevant though.

Comment 89

[Jason Smith [:jsmith]]

Actually, just found

Comment 90

[Brian R. Bondy [:bbondy]]

jsmith another thing to add to the test plan is to make sure that Windows doesn't overwrite the files I'm replacing as 0 byte read only files.  Even after a reboot or any other case you can think of.

Comment 91

[Jason Smith [:jsmith]]

(In reply to Brian R. Bondy [:bbondy] from comment #90)
> jsmith another thing to add to the test plan is to make sure that Windows
> doesn't overwrite the files I'm replacing as 0 byte read only files.  Even
> after a reboot or any other case you can think of.

Right. Something I was thinking about - Doesn't this patch generally only work for users who have access to C:/Windows/Prefetch in the first place? I just noticed I was on my guest account for my work machine (Win 7 64-bit), which did not have access to C:/Windows/Prefetch. Although the modification to C:/Windows/Prefetch only needs to happen once on a machine entirely, so I guess as long as the admin jumps on a firefox build eventually that supports this patch, then the prefetch files will eventually be eliminated (which is likely).

Comment 92

[Brian R. Bondy [:bbondy]]

For the clearing of prefetch that happens on updates currently yes but bug 758463 will fix that.

For the clearing of prefetch that happens one time after 3 minutes, it will clear no matter what account type you are.  Because it asks the service to do the clearing and any account type that is a user can issue such a command to the service.

Comment 93

[Brian R. Bondy [:bbondy]]

Hey jsmith, here are some tests that I ran to ensure I can land on Nightly, just including them below in case they are helpful for your test plan.
This is not meant to be a conclusive list of what to test, but should provide some insight and ideas.
I wanted to land this tonight but I found Bug 758998 which should be fixed first. 

Tests I ran from a clean machine in a state that hasn't tested prefetch before:

1) XP, Vista, Win7: Test an upgrade from a changeset that doesn't have prefetch to a changeset that does have prefetch
- Make sure upgrade works
- Make sure service logs look normal
- Make sure there is no message in logs about prefetch being cleared (maintenanceservice-install.log)
- Make sure prefetch files are not cleared
- Make sure after 3 minutes that prefetch operation is run and prefetch gets cleared
- Test to make sure if prefetch files are already cleared and you run the one time 3 minute operation it indicates that they are already cleared in the log for maintenanceservice.log

2) XP, Vista, Win7: Test an upgrade form a changeset that does have prefetch to another changeset that does have prefetch
- Make sure upgrade works
- Make sure prefetch gets cleared after the update
- Make sure service logs look normal and maintenanceservice-install.log indicates prefetch is cleared, and check windows/prefetch to ensure it is cleared.
- Make sure after 3 minutes that prefetch operation is run and prefetch gets cleared (note pref was reverted before this test)
 
3) XP, Vista, Win7: Test that using an old service version does not cause problems for upgrades
- Install a build that has prefetch clearing
- Uninstall the Mozilla Maintenance Service
- Install the mozilla maintenance service from a branch that doesn't yet have prefetch clearing, example run: "C:\Program Files (x86)\Aurora\maintenanceservice_installer.exe"
- Test to make sure an upgrade works
- Test to make sure the one time clear operation simply indicates "Service command not recognized: clear-prefetch." in maintenanceservice.log
- Repeat this test with a service version that had background updates in it but not prefetch.

4) XP, Vista, Win7: Make sure Windows never overwrites the 0 bytes read only files

5) Make sure when prefetch files are all cleared that this DOWRD value existsa nd it is set to 1: 
HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService\FFPrefetchDisabled
- Make sure that the registry location doesn't exist when a clear operation happend, but there are no files to clear.
- Make sure that the registry location doesn't exist when a prefetch file can't be cleared, try setting it to readonly but not a 0 byte file to restrict clearing it
 
Other notes:
- Update clearing will not work on XP because of bug 758463
- To revert back to the pre-prefetch state, i) reset app.update.service.prefetchCleared in about:config, ii) Unzip prefetch files to their previously backed up state
- Tests should be done both when prefetch files exist and when they do not yet exist.

Comment 94

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/4dd2d5f25910

Comment 95

[Danial Horton]

"I'm a bit reluctant to findfile() in the huge prefetch directory as that might be expensive(but we can just measure that with telemetry)."

huge? the prefetch folder (when working properly) should only list 128 files at a maximum.

Comment 96

[Brian R. Bondy [:bbondy]]

Also there' no alternative and it doesn't matter because the service does this operation out of process.

Comment 97

[Brian R. Bondy [:bbondy]]

The findfile discussion is a bit out of context anyway with the current implementation.

Comment 98

[Danial Horton]

(In reply to Taylor Cheetham from comment #43)
> I am worried that this could actually hurt performance in later windows
> versions, namely windows 8. Windows 8 now uses all available free ram as
> cache (as far as i am aware win7 only used some)until otherwise needed, i
> have been testing windows 8 for awhile now and ff opening even from a
> restored session has been near instant. If i didnt know better i would even
> go so far as to say it had been minimized, however i am not sure what
> windows 8 uses to determine what should and should not be in this cache. 

Windows 7 also uses all available free ram as a file cache based on contents of the layout.ini and readyboot prefetch database.
Deleting prefetch files won't change how readyboots database forms.

the contents of the pf file are simple indexs to file location and contents, so deleting the firefox pf file won't hurt readyboot caching (take note of the fact i say readyboot and not readyboost, readyboot is the core service behind superfetch and readyboost tech)

Comment 99

[Brian R. Bondy [:bbondy]]

>  ...also uses all available free ram...

Good to know, we're only trying to reduce the file IO for this task so that our preload code which seems to perform better than having .pf files can run.

Comment 100

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

:jesse is suggesting we should do a security code review of this patch

Jesse:
[09:47:24] curtisk|afk: i think this should get some extra code review, since it's part of the privileged maintenance service and is doing fairly low-level things with strings and filesystems http://hg.mozilla.org/mozilla-central/rev/4dd2d5f25910#l6.68

Comment 101

[Brian R. Bondy [:bbondy]]

sounds good, thanks!

Comment 102

[Jason Smith [:jsmith]]

Verified that this code has landed. Follow-up bugs will be filed to track any issues that need to be resolved.

Comment 103

[Brian R. Bondy [:bbondy]]

Other bugs posted after this completely remove all code in this patch on all branches so no security review is needed here.