320328 - Ts regression from bug 316416 (nsIModuleLoader)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Benjamin Smedberg]

There was a Ts regression on some machines from bug 316416, although the Ts numbers are wonky enough that I'm not sure I can trust them. The two tboxen that showed a definite reaction were seamonkey monkey and creature: the camino tbox wanders around aimlessly and all the others show a flat line or even improvement.

Right now I'm suspecting calls to nsLocalFile::Equals on both mac and windows: they both hit the filesystem for both localfiles being compared and don't cache the result.

On windows my attempted solution is going to be to cache the shortpath instead of the hash.

On mac my attempted solution is to use GetFSRefInternal with bForceUpdateCache = PR_FALSE so that we use the cached FSRef when available.

Comment 1

[Benjamin Smedberg]

Attachment: Cache the shortname, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Use the cached FSRef

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

nsLocalFileWin::Equals
+    nsCOMPtr<nsILocalFileWin> lf(do_QueryInterface(inFile));
+    if (!lf)
+        return NS_NOINTERFACE;

shouldn't that just set *_retval to false and return success?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

+nsLocalFile::EnsureShortPath()
+{
+    if (mShortWorkingPath.Length())

if (!mShortWorkingPath.IsEmpty())

Comment 5

[Simon Fraser [no longer active]]

Comment on attachment 206012 [details] [diff] [review]
Use the cached FSRef

Josh, since you were worried about nsLocalFile caching, can you look at this?

Also, do we have any startup numbers with this patch?

Comment 6

[Benjamin Smedberg]

No, my only mac is a laptop which is not stable enough to produce useful Ts numbers.

Comment 7

[Josh Aas]

Comment on attachment 206012 [details] [diff] [review]
Use the cached FSRef

smfr: I was worried about the fact that Windows and Linux cache file sizes and don't update the cache when a file is written to. Apparently we're going to live with that bug (*smacks forehead*). This doesn't seem to have anything to do with that.

This looks fine to me. If Ts doesn't improve though, I suggest just backing it out since it is an unnecessary risk.

Comment 8

[Darin Fisher]

Comment on attachment 205967 [details] [diff] [review]
Cache the shortname, rev. 1

>Index: xpcom/io/nsILocalFileWin.idl

>+    /**
>+     * The native short pathname (8.3) for this file. If there is no
>+     * short path, the long path will be returned.
>+     */
>+    readonly attribute AString shortPath;
>+    [noscript] readonly attribute ACString nativeShortPath;

It seems to me that exposing the short path is just not that ideal b/c
of the fact that there simply may be no short path on some filesystems.
This patch only needs a canonical path that can be hashed and compared
for equality.  So, it would seem that exposing a "canonicalPath" 
attribute might be best.  Also, why aren't you just using the 
persistent descriptor attribute for this purpose?

Comment 9

[Benjamin Smedberg]

We (Silver) already vetoed using the persistent descriptor because it currently is displayed in the Firefox pref UI and is typically the long name.

How is your "canonicalPath" attribute different than the "shortPath" I created?

Comment 10

[Darin Fisher]

> We (Silver) already vetoed using the persistent descriptor because it 
> currently is displayed in the Firefox pref UI and is typically the long name.

So, why not use the long name as the canonical name?  You can call GetLongPathName on Win98 and above.  On Win95, you'd have to call FindFirstFile on each path segment to normalize to the long path, but we weren't planning on supporting Win95 for FF 3, right?


> How is your "canonicalPath" attribute different than the "shortPath" I created?

I think my point was that you just need some canonical path to hash and compare, you don't need it to be the short version of the path, which may not exist on some systems, so why call it the short path?  I'm arguing for renaming the attribute at least.

Comment 11

[Benjamin Smedberg]

Attachment: Cache the shortname, rev. 1.1

I'll cave.

Comment 12

[Benjamin Smedberg]

Comment on attachment 208103 [details] [diff] [review]
Cache the shortname, rev. 1.1

Actually, let me fix biesi's nits.

Comment 13

[Benjamin Smedberg]

Attachment: Cache the shortname, rev. 1.2

Comment 14

[Darin Fisher]

Comment on attachment 208103 [details] [diff] [review]
Cache the shortname, rev. 1.1

>Index: xpcom/io/nsILocalFileWin.idl

>+    /**
>+     * The canonical path of the file, which avoids short/long pathname
>+     * inconsistencies.
>+     */
>+    readonly attribute AString canonicalPath;
>+    [noscript] readonly attribute ACString nativeCanonicalPath;

I think it'd be good to add some comment to explain why this is not
the same as nsILocalFile::persistentDescriptor.  To the casual observer,
a persistent descriptor would seem to be something that uniquely identifies
the file path, but the thing that may not be obvious is that the persistent
descriptor need not be canonicalized.

You should probably explain that the format of the canonical path may vary
from (file)system to (file)system.  For example, on some (file)systems, the
value is the short path name, and on others it is the long path name.

Comment 15

[Benjamin Smedberg]

Attachment: Cache the shortname, rev. 1.3

Update comments

Comment 16

[Darin Fisher]

Comment on attachment 208106 [details] [diff] [review]
Cache the shortname, rev. 1.3

r=darin

Comment 17

[Benjamin Smedberg]

Fixed on trunk.