Closed Bug 917012 Opened 6 years ago Closed 6 years ago

too many mainthread stat() calls during downloads

Categories

(Toolkit :: Downloads API, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: taras.mozilla, Assigned: Paolo)

References

Details

Attachments

(1 file)

(In reply to Marco Castelluccio [:marco] from comment #1)
> Bug 827010?

There is no reason to do any syscalls here. We should have file size in gecko already.
(In reply to Taras Glek (:taras) from comment #2)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> > Bug 827010?
> 
> There is no reason to do any syscalls here. We should have file size in
> gecko already.

err...we should know that the file exists by nature of it being downloaded :)
I should note that I had the downloads window open in background...but was interacting with main window and hitting these .exists janks
From an IRC discussion:
<gavin> seems to be a dupe of bug https://bugzilla.mozilla.org/show_bug.cgi?id=827010
<firebot> Bug 827010 nor, --, ---, nobody, NEW, In download views, use async I/O for retrieving data about the target and part files of downloads
<gavin> referenced in the code
<taras> gavin: right, except for the fact that we shouldnt stat() things we know the size of
<gavin> well it's not about knowing the size
<gavin> it's about the existence on disk
<gavin> we hide/disable the "Open file" menu option if you've since deleted the downloaded file, e.g.
<taras> gavin: sorry, same thing :)
<taras> we know a file exists if we are downloading it
<taras> :)
<gavin> this code shouldn't be called during a download
<gavin> well, I guess it could, if that results in selection changes
<taras> gavin: it's called periodically
<taras> without any interactions with download stuff
<gavin> that seems like the bug
This happens when the state of a download changes, if the Library window or the in-content Downloads view is open. Bug 827010 was filed to fix that, assuming the file exists right after completion, and using OS.File to check for its existence later.
Blocks: 907082
Attached patch The patchSplinter Review
It turned out that bug 827010 was exacerbated by the fact that we sent
onStateChange notifications too often. This patch fixes the regression
introduced by the new Downloads API, while it leaves the general stat()
call issue to bug 827010. It might be that that bug will be addressed as
part of the refactoring to use the Downloads API without DataItem wrappers.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #809874 - Flags: review?(enndeakin)
Attachment #809874 - Flags: review?(enndeakin) → review+
Summary: Pointless mainthread stat() calls during downloads → too many mainthread stat() calls during downloads
https://hg.mozilla.org/mozilla-central/rev/01de55e55a15
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 809874 [details] [diff] [review]
The patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 847863
User impact if declined: Severe jank when Library window open during downloads
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Tested manually
String or IDL/UUID changes made by this patch: None
Attachment #809874 - Flags: approval-mozilla-aurora?
Attachment #809874 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.