Closed Bug 455828 Opened 11 years ago Closed 11 years ago

get rid of FSRef caching in mac file code

Categories

(Core Graveyard :: File Handling, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix v1.0 (obsolete) — Splinter Review
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+
Attached patch fix v1.1Splinter Review
Attachment #339194 - Attachment is obsolete: true
Attachment #339305 - Flags: superreview?(doug.turner)
Blocks: 322686
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+
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.
landed on trunk
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.