Closed Bug 1614659 Opened 10 months ago Closed 8 months ago

CacheIndex::Shutdown() should not check if files exist before removing them

Categories

(Core :: Networking: Cache, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: florian, Assigned: michal)

Details

(Whiteboard: [necko-triaged][fxperf:p2])

Attachments

(1 file)

During shutdown, <profile>/cache2/index.tmp and <profile>/cache2/index.log are stat'ed before removing them (if they exist). This code should just remove the files without checking first if they exist (and decide based on the failure code returned by remove if we failed to remove them because they didn't exist, or due to another error that might be worth logging).

This is the code at https://searchfox.org/mozilla-central/rev/a1592902acabf9bded973067133baaac1457f3d3/netwerk/cache2/CacheIndex.cpp#1873-1878,1898,1900-1901

Profile where I noticed this: https://perfht.ml/31KKPAd

The stack is:

nsXREDirProvider::DoShutdown() [xul.dll]
nsObserverService::NotifyObservers profile-before-change []
nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) [xul.dll]
nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) [xul.dll]
mozilla::net::CacheObserver::Observe(nsISupports*, char const*, char16_t const*) [xul.dll]
static mozilla::net::CacheFileIOManager::Shutdown() [xul.dll]
static mozilla::net::CacheIndex::Shutdown() [xul.dll]
mozilla::net::CacheIndex::RemoveJournalAndTempFile() [xul.dll]
mozilla::net::CacheIndex::RemoveFile(nsTSubstring<char> const&) [xul.dll]
nsLocalFile::Exists(bool*) [xul.dll]
nsLocalFile::ResolveAndStat []
nsLocalFile::ResolveAndStat() [xul.dll]
GetFileInfo(nsTString<char16_t> const&, PRFileInfo64*) [xul.dll]
GetFileAttributesExW [KERNELBASE.dll]

If there's a way for this code to not need to touch these files at all during shutdown, that would be even better of course.

Honza, can you take a look?

Flags: needinfo?(honzab.moz)
Assignee: nobody → michal.novotny
Flags: needinfo?(honzab.moz)
Priority: -- → P2
Whiteboard: [fxperf] → [necko-triaged][fxperf]
Whiteboard: [necko-triaged][fxperf] → [necko-triaged][fxperf:p2]

hey @michal
Can I take this up ?
Thanks :)

Sure

hey [:michal] !!
It seems that In this I need to remove these lines(https://searchfox.org/mozilla-central/rev/a1592902acabf9bded973067133baaac1457f3d3/netwerk/cache2/CacheIndex.cpp#1873-1878) which check the files whether they exist or not .
I want to ask whether I'm heading in the right direction or not ?
Thanks

Yes, this means removing lines 1873-1877 (not 1878). You also need to decide what to return if file->Remove() fails, so you need to check all code paths which end up calling CacheIndex::RemoveFile(). Also change the LOG message, because now it sounds like an error, but it will no longer be true.

Hey [:michal]
I have one doubt .It is written in comment 1 that files index.tmp(i.e TEMP_INDEX_NAME) and index.log(i.e JOURNAL_NAME) are stat'ed before removing them (if they exist ). so do I need to do this for these files only by comparing aName in RemoveFile with their respectives names?
And you said to change the lOG message ,How should I change that ?
So here is my Approach for this :
After removing lines 1873-1877

rv = file->Remove(false);
if(NS_Failed(rv))
 {
    if(rv == NS_ERROR_FILE_NOT_FOUND)
         return rv;
    else
        {
             NS_WARNING("It failed because of another error that might need worth logging");
            //here I can't figure out what to return ?
         }
}
return NS_OK

(In reply to Mahak from comment #6)

Hey [:michal]
I have one doubt .It is written in comment 1 that files index.tmp(i.e TEMP_INDEX_NAME) and index.log(i.e JOURNAL_NAME) are stat'ed before removing them (if they exist ). so do I need to do this for these files only by comparing aName in RemoveFile with their respectives names?

No, it should be fine to skip the check for all files.

And you said to change the lOG message ,How should I change that ?

If we can rely on NS_ERROR_FILE_NOT_FOUND, then you don't have to change the message and just skip logging in case of this error code. In any case, add error code to the message just for case that nsLocalFile::Remove() throws some unexpected error code on some platform.

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07440613e13a
CacheIndex::Shutdown() should not check if files exist before removing them.r=michal
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

== Change summary for alert #25599 (as of Sun, 12 Apr 2020 22:47:05 GMT) ==

Improvements:

44% build times windows2012-64 debug taskcluster-c5.4xlarge 1,339.11 -> 744.07
43% build times linux64 debug base-toolchains taskcluster-m5.4xlarge 2,275.03 -> 1,298.41
42% build times linux64 asan opt taskcluster-m5.4xlarge 1,434.68 -> 831.06
40% build times windows2012-aarch64 debug aarch64 taskcluster-m5.4xlarge 1,121.28 -> 676.72
39% build times android-5-0-aarch64 opt gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,536.02 -> 932.54
39% build times android-4-0-armv7-api16 opt gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,558.12 -> 951.08
38% build times android-4-2-x86 opt gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,550.59 -> 955.52
38% build times android-5-0-x86_64 opt gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,571.72 -> 979.59
37% build times android-5-0-x86_64 opt taskcluster-c5.4xlarge 1,324.48 -> 837.99
36% build times linux64 opt taskcluster-c5d.4xlarge valgrind 1,507.19 -> 959.82
33% build times android-4-0-armv7-api16 debug taskcluster-c5d.4xlarge 1,148.06 -> 773.78
30% build times android-5-0-aarch64 debug gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,425.27 -> 996.67
30% build times android-4-0-armv7-api16 debug gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,456.42 -> 1,022.34
28% build times linux64 debug gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,648.73 -> 1,194.77
27% build times osx-cross debug gcp taskcluster-projects/970387039909/machineTypes/custom-32-73728 1,823.76 -> 1,335.78

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25599

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