Closed Bug 308369 Opened 19 years ago Closed 19 years ago

Stop copying stat cache when cloning nsIFile instances

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta5

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

(Keywords: fixed1.8)

Attachments

(1 file, 1 obsolete file)

v1.1 patch
4.68 KB, patch
jaas
: review+
dougt
: superreview+
asa
: approval1.8b5+
Details | Diff | Splinter Review
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.
-> me
Status: NEW → ASSIGNED
Flags: blocking1.8b5?
Target Milestone: --- → mozilla1.8beta5
Attached patch v1 patch (obsolete) — — Splinter Review
Attachment #195930 - Flags: superreview?(dougt)
Attachment #195930 - Flags: review?(joshmoz)
Assignee: dougt → darin
Status: ASSIGNED → NEW
Blocks: 308370
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 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)
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.
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.
Attached patch v1.1 patch — — Splinter Review
bye bye memset
Attachment #195930 - Attachment is obsolete: true
Attachment #195941 - Flags: superreview?(dougt)
Attachment #195941 - Flags: review?(joshmoz)
Attachment #195930 - Flags: superreview?(dougt)
Attachment #195930 - Flags: review?(joshmoz)
Attachment #195941 - Flags: review?(joshmoz) → review+
Attachment #195941 - Flags: superreview?(dougt) → superreview+
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #195941 - Flags: approval1.8b5?
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.
(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."
Thanks Darin. That bug has been marked as a 1.8b5 blocker so this bug must be too.
Flags: blocking1.8b5? → blocking1.8b5+
Attachment #195941 - Flags: approval1.8b5? → approval1.8b5+
fixed1.8
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: