The default bug view has changed. See this FAQ.

Stop copying stat cache when cloning nsIFile instances

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta5
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
Stop copying stat cache when cloning nsIFile instances

I now believe that this patch was a mistake.  See discussion in bug 307815 and
bug 307429.  Caching stat buffers is a real big hack that results in a variety
of workaround hacks.  The ability to dump the cache by cloning a nsIFile is
nice.  The alternative is to create a new nsIFile and initialize it via
initWithFile, but that is more work for consumers.  We should make nsIFile.clone
and nsILocalFile.initWithFile work consistently in this regard.
(Assignee)

Comment 1

12 years ago
-> me
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
Target Milestone: --- → mozilla1.8beta5
(Assignee)

Comment 2

12 years ago
Created attachment 195930 [details] [diff] [review]
v1 patch
Attachment #195930 - Flags: superreview?(dougt)
Attachment #195930 - Flags: review?(joshmoz)
(Assignee)

Updated

12 years ago
Assignee: dougt → darin
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Blocks: 308370

Comment 3

12 years ago
Comment on attachment 195930 [details] [diff] [review]
v1 patch

do you really neet to zero the mFileInfo64?  I know it is being done in other
places.
Attachment #195930 - Flags: superreview?(dougt) → superreview+

Comment 4

12 years ago
Comment on attachment 195930 [details] [diff] [review]
v1 patch

Why the memset calls? Also, are we just letting the perf win from 122892 go
away?
Attachment #195930 - Flags: superreview+ → superreview?(dougt)

Comment 5

12 years ago
I think I accidently killed dougt's sr+ since both of us were editing the
attachment at the same time and he hit submit first. I'd add my r+, but I don't
see why we need the memset calls that zero mFileInfo64.
(Assignee)

Comment 6

12 years ago
I added the memset calls for consistency with the default constructors.  If we
want to drop memset from the copy constructors then we should do so for the
default constructors as well.  I suspect that there is no reason for the memset
calls, otherwise.

As for the performance impact of this change, I don't think it's significant
enough to concern us with the impending release.
(Assignee)

Comment 7

12 years ago
Created attachment 195941 [details] [diff] [review]
v1.1 patch

bye bye memset
Attachment #195930 - Attachment is obsolete: true
Attachment #195941 - Flags: superreview?(dougt)
Attachment #195941 - Flags: review?(joshmoz)
(Assignee)

Updated

12 years ago
Attachment #195930 - Flags: superreview?(dougt)
Attachment #195930 - Flags: review?(joshmoz)

Updated

12 years ago
Attachment #195941 - Flags: review?(joshmoz) → review+

Updated

12 years ago
Attachment #195941 - Flags: superreview?(dougt) → superreview+
(Assignee)

Comment 8

12 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #195941 - Flags: approval1.8b5?

Comment 9

12 years ago
Darin, can you elaborate more for the triage team about why we should take this
fix for Firefox 1.5? What's the user impact, it's unclear from the bug? Thanks.
(Assignee)

Comment 10

12 years ago
(In reply to comment #9)
> Darin, can you elaborate more for the triage team about why we should take this
> fix for Firefox 1.5? What's the user impact, it's unclear from the bug? Thanks.

Scott: This patch is needed in order to fix bug 307429 "properly."

Comment 11

12 years ago
Thanks Darin. That bug has been marked as a 1.8b5 blocker so this bug must be too.
Flags: blocking1.8b5? → blocking1.8b5+

Updated

12 years ago
Attachment #195941 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 12

12 years ago
fixed1.8
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.