724203 - Optimize nsLocalFile::IsDirectory on Windows by 50% giving 5ms startup improvement

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brian R. Bondy [:bbondy]]

Attachment: Performance results

IsDirectory takes ~10ms at startup time, after this patch it takes ~5ms.

This is only a small portion of startup time but it still helps.  I gave the about:startup results in a screenshot attached, but the differences may just be to variance. 

The reason for the improvement is because we were calling GetFileAttributesEx which is significantly slower than GetFileAttributes.

The extra info returned by GetFileAttributesEx is not needed from this function call.

I made a new Resolve() function that I'll try to use in place of ResolveAndStat where applicable for other work I plan to do in nsLocalFileWin. 

The new Resolve() function is almost free, ResolveAndStat is slow because of the stat part (which calls GetFileAttributesEx).

This function is often called on the main thread and throughout all code so it also helps other aspects than startup.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Comment 2

[[e-mail]]

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

A quick glance at nsLocalFileWin.cpp suggests that there are other cases where ResolveAndStat() could be replaced with Resolve(). There was one other user that only needs to know whether the target is a file but that could probably be modified to use IsFile().

>+ * Fills the mResoledPath member variable with the file or symlink target
Typo: Resol(v)ed

>+  // if we aren't dirty then we are already done
>+  if (!mResolveDirty) {
ResolveAndStat doesn't clear this, so you might do extra work in some cases.
I was trying to work out whether MakeDirty should clear mWorkingPath and Resolve should test for that instead, but that would mean having to change the unusual behaviour when resolving an invalid .lnk file, and I have no idea whether anyone relies on it.

>+  // hack from ResolveAndStat designed to work around bug 134796 
>+  // until it is fixed.
>+  nsAutoString nsprPath(mWorkingPath.get());
>+  if (mWorkingPath.Length() == 2 && mWorkingPath.CharAt(1) == L':') 
>+      nsprPath.AppendASCII("\\");
You don't actually use the nsprPath...

>+  nsresult rv = ResolveShortcut();
This will fail if the file does not exist, although I think that's OK here.

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks for the passes Neil.

> A quick glance at nsLocalFileWin.cpp suggests that there 
> are other cases where ResolveAndStat() could be replaced with Resolve(). 
> There was one other user that only needs to know whether the target is a file 
> but that could probably be modified to use IsFile().

Ya I suspect there is as well, I'm taking them one opt at a time as I find them. I suspect there are 1 or 2 left in nsLocalFileWin that I haven't gotten to yet.  Exists is only 1ms of startup time but it helps non startup time too so I'll likely update that one as well.

> ResolveAndStat doesn't clear this, so you might do extra work in some cases.

I had a dream about that last night and was going to update that to potentially save some work this morning :P  I'll provide an updated patch for setting mResolvedDirty to false when ResolveAndStat is called.

> This will fail if the file does not exist, although I think that's OK here.

Yup that's ok.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

bsmedberg, marking this for Neil since he already looked at it. Please let me know if you need to sr it.

Implemented review comments.

Comment 5

[[e-mail]]

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

r=me assuming that you follow up to ensure that we aren't calling ResolveAndStat in other places that we don't need to.

Comment 6

[Brian R. Bondy [:bbondy]]

> r=me assuming that you follow up to ensure that we aren't calling 
> ResolveAndStat in other places that we don't need to.

yup will be finishing the opts in this file probably by next week.  Thanks for the review.

Comment 7

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/475090373d31

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/475090373d31