If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add ability to hash on nsIFiles with nsIHashable

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPCOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
I need to be able to hash on nsILocalFiles, I've got a nsFileHashKey
(Assignee)

Comment 1

12 years ago
Created attachment 203272 [details] [diff] [review]
Hash on persistentDescriptor and make the PD/nativePath the short pathname on windows when practical

The bit about making the windows nsIFile.nativePath return the shortname when possible is a bit scary... I could do this just for the persistentdescriptor (PD forwards to GetNativePath currently).
Attachment #203272 - Flags: review?(darin)

Comment 2

12 years ago
Comment on attachment 203272 [details] [diff] [review]
Hash on persistentDescriptor and make the PD/nativePath the short pathname on windows when practical

> nsLocalFile::GetNativePath(nsACString &_retval)
>+    DWORD thisr = GetShortPathName(mWorkingPath.get(),
>+                                   thisshort, sizeof(thisshort));

Eeep. Could you explain why you need to change the PD to short paths?

Comment 3

12 years ago
Yeah, I'd like to know why too.  My initial reaction is "no way!"  Perhaps you meant to modify what nsILocalFile::persistentDescriptor returns?

Comment 4

12 years ago
I'm not sure I could even justify changing only the PD, depending on who uses it for what.
(Assignee)

Comment 5

12 years ago
That was my other alternative, sure... Silver, I do need to change persistentDescriptor so that it will hash properly, though I don't necessarily need to change the nativePath.

Comment 6

12 years ago
Why can't it hash the real path "properly"? Is that a defect of the hash code, or are you trying to canonicalise the path?

My concern with even just the PD changing is that initialising a nsILocalFile using the PD [1] will not correct the short path into the real path - which will, I believe, make all files set in Firefox's pref window go to their short form (which is clearly unacceptable).

[1] http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#322

Comment 7

12 years ago
If we had the freedom to modify nsILocalFile, then I'd want to add a hashCode getter to it ;-)

persistentDescriptor is a value defined to be opaque to the consumer of nsILocalFile.  It isn't even guaranteed to be normalized to anything that could be used as a key into a map, but I think it's reasonable to introduce that as a feature of the frozen API.  Given that no one should be implementing nsILocalFile outside of XPCOM, it is likely an OK change to make.

We do have to ensure that SetPersistentDescriptor still understands the old format as well as the new.  So, I agree with James... we'd need to fix it to unshorten the path, but that is hard to do without unshortening things that wouldn't have been unshortened before.

Another issue is that it is possible to disable short file paths on Windows, so we would not want to persist a file path to disk that could not be interpreted properly later-on if someone toggled that OS setting.
(Assignee)

Comment 8

12 years ago
Yes, I need to normalize so that a nsIFile with a shortname and a nsIFile with a long name hash the same (see nsLocalFileWin::Equals for equivalent code to compare the two). If normalizing to the longname would be better it looks to be just as easy.

Comment 9

12 years ago
Normalizing to the long name would probably be fine...

But, maybe the better answer is a new interface: nsISupportsHashCode

You could probably invent a nsSupportsHashKey that would QI its keys to that interface to get a hashCode value for the object.
(Assignee)

Comment 10

12 years ago
Really I would want to normalize windows nsIFile to the longname early... when you set nativePath or initWithPath or whatever. It's a change in behavior, but it certainly doesn't break the API docs...

If we invented a new nsIHashable interface, who else would use it? I presume it would have Equals() and hashCode as the two methods.

Comment 11

12 years ago
> If we invented a new nsIHashable interface, who else would use it? I presume it
> would have Equals() and hashCode as the two methods.

URI objects come to mind... though getting the "spec" of a nsIURI is known to return a normalized string, and we have optimized that code path to ensure that there isn't any string copying.  You also couldn't assume that every nsIURI QIs to nsIHashable since nsIURI may be implemented by a third-party protocol handler.

I can imagine other object types wanting something like this though.

Comment 12

12 years ago
Comment on attachment 203272 [details] [diff] [review]
Hash on persistentDescriptor and make the PD/nativePath the short pathname on windows when practical

OK, I'm certain that we don't want to change GetNativePath like this.
Attachment #203272 - Flags: review?(darin) → review-
remember that the short name may not exist in some cases.
also, why not hash the .path of the file, and making that return the long path? that avoids changes to the persistent descriptor.
(Assignee)

Comment 15

12 years ago
The .path is supposedly not a guarantee of uniqueness on mac, or something like that?
oh, right...
(Assignee)

Comment 17

12 years ago
Created attachment 203430 [details] [diff] [review]
nsIHashable

This does the nsIHashable approach, because it turns out that the GetLongPathName function is not implemented on win95 or NT4 (it is in win98/win2k, but I think we still support NT4). Very recent MS SDKs have a workaround of sorts, but it's also not clear that we can rely on those SDKs.
Attachment #203430 - Flags: review?(darin)

Comment 18

12 years ago
Comment on attachment 203430 [details] [diff] [review]
nsIHashable

>Index: xpcom/ds/nsIHashable.idl

>+
>+[scriptable, uuid(17e595fa-b57a-4933-bd0f-b1812e8ab188)]
>+interface nsIHashable : nsISupports

nit: please add a comment block for this interface.


>Index: xpcom/glue/nsHashKeys.h

>+    static PLDHashNumber HashKey(const nsIHashable* aKey) {
>+        PRUint32 code = 8888; // magic number if GetHashCode fails :-(
>+        NS_CONST_CAST(nsIHashable*,aKey)->GetHashCode(&code);
>+        return code;

nit: Maybe capture the nsresult return code from GetHashCode and
assert that it isn't a failure code?



>Index: xpcom/io/nsFileHashKey.h
...
>+class nsFileHashKey : public PLDHashEntryHdr

Huh?  Do we still want this?


>Index: xpcom/io/nsLocalFileWin.cpp

>+NS_IMETHODIMP
>+nsLocalFile::GetHashCode(PRUint32 *aResult)
>+{
>+    char thisshort[MAX_PATH];
>+    DWORD thisr = GetShortPathName(mWorkingPath.get(),
>+                                   thisshort, sizeof(thisshort));
>+    if (thisr < sizeof(thisshort))
>+        *aResult = nsCRT::HashCode(thisshort);
>+    else
>+        *aResult = nsCRT::HashCode(mWorkingPath.get());
>+
>+    return NS_OK;
>+}

nit: Please add a comment explaining why GetShortPathName is being used here.


r=darin assuming nsFileHashKey is not part of the patch
Attachment #203430 - Flags: review?(darin) → review+
(Assignee)

Comment 19

12 years ago
Fixed on trunk. mkaply, nsLocalFileOS2 is going to need similar changes in order for bug 316416 to work on OS/2.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: Add nsFileHashKey → Add ability to hash on nsIFiles with nsIHashable
(Assignee)

Comment 20

12 years ago
Argh, wrong kaply.

Comment 21

12 years ago
(In reply to comment #19)
> Fixed on trunk. mkaply, nsLocalFileOS2 is going to need similar changes in
> order for bug 316416 to work on OS/2.
> 

Done.

Incidentally, that other kaply is my bro :)
(Assignee)

Comment 22

12 years ago
Turns out that using this on windows is a serious perf issue because getting shortpathname causes stat()s which are painfully slow. We've talked about cacheing the stat result.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

12 years ago
Created attachment 205256 [details] [diff] [review]
Cache the hash on windows, rev. 1
Attachment #203272 - Attachment is obsolete: true
Attachment #205256 - Flags: review?(darin)

Comment 24

12 years ago
Comment on attachment 205256 [details] [diff] [review]
Cache the hash on windows, rev. 1

r=darin
Attachment #205256 - Flags: review?(darin) → review+
(Assignee)

Comment 25

12 years ago
Cached hash landed on trunk.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.