bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

get rid of FSRef caching in mac file code

RESOLVED FIXED

Status

Core Graveyard
File Handling
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.05 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 339194 [details] [diff] [review]
fix v1.0

We should probably get rid of FSRef caching in mac files code. It doesn't stay valid in the face of external file modifications, see bug 322686. I doubt the cache saves us much time anyway, in my tests there is no Ts or Tp hit when it is removed.
Attachment #339194 - Flags: review?(mstange)
Comment on attachment 339194 [details] [diff] [review]
fix v1.0

(Side note: It would be cool if this came with more context and function names after the @@.)

Looks good to me, I think this patch is the way to go. An incorrect cache is of no use.
I looked at bug 164396 which added the cache, and the problem of invalid FSRefs because of external changes has apparently not been addressed at all...

r=me if you remove this change (there's a return in the if):

-  // CFURLGetFSRef only returns a Boolean for success,
-  // so we have to assume what the error was. This is
-  // the only probable cause.
-  return NS_ERROR_FILE_NOT_FOUND;
+  else {
+    // CFURLGetFSRef only returns a Boolean for success,
+    // so we have to assume what the error was. This is
+    // the only probable cause.
+    return NS_ERROR_FILE_NOT_FOUND;
+  }

Or was there a reason for this change?
Attachment #339194 - Flags: review?(mstange) → review+
(Assignee)

Comment 2

10 years ago
Created attachment 339305 [details] [diff] [review]
fix v1.1
Attachment #339194 - Attachment is obsolete: true
Attachment #339305 - Flags: superreview?(doug.turner)
(Assignee)

Updated

10 years ago
Blocks: 322686

Comment 3

10 years ago
Comment on attachment 339305 [details] [diff] [review]
fix v1.1

diff -r dabae13daacb xpcom/io/nsLocalFileOSX.h

Lose the extra space before the new line after EqualsInternal



Did you run ts?  does this cost anything?  if not, looks good;
Attachment #339305 - Flags: superreview?(doug.turner) → superreview+
(Assignee)

Comment 4

10 years ago
I ran perf tests and didn't see any difference with this patch. As the patch that added caching in the first place noted, the caching isn't actually used that often.
(Assignee)

Comment 5

10 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.