Closed
Bug 502694
Opened 15 years ago
Closed 14 years ago
Images should not have individual discard timers
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla2.0b2
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: jrmuizel, Assigned: bholley)
References
Details
(Keywords: perf, Whiteboard: [Tsnap])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•15 years ago
|
||
I'm guessing there was some offline discussion about this bug, but could you clarify the intention a little bit? Is the idea to have one timer that periodically sweeps through the images and discards old ones?
Comment 2•15 years ago
|
||
It's not 100% clear. Jeff just wants us to stop having 1 timer per image; how we accomplish that is up to us.
I, too, would like us to stop having 1 timer per image. Also I would like to have us stop destroying a timer and then creating a new one each time it needs to be reset, but that would be fixed if we used a different approach. Bug 506817 isn't quite a dup of this; I think what rob was seeing there was pathological timer firing even when it didn't need to be, but regardless this should fix that as well.
Flags: blocking1.9.2?
Updated•15 years ago
|
Whiteboard: [Tsnap]
Assignee | ||
Comment 5•15 years ago
|
||
on my post bug 435296 todo list. I'll get to it mid-to-late august.
Assignee | ||
Comment 6•15 years ago
|
||
one thing that should possibly be considered when reworking discard triggering is having the cache trigger a discard when the image has no proxies. This should be pretty simple to do once bug 435296 lands.
Assignee | ||
Comment 7•15 years ago
|
||
Doing analysis for bug 516331, and finding out that this is a pretty significant perf hit. Disabling the resetting of discard timers alone is at least a 3% Tp win, possibly 7%. (the 3% case was on a smaller pageset, but I controlled the experiment by setting the discard timer impossibly high in both runs, so the handler routine would never be invoked. the 7% was on Tp4, but I didn't fix the timer).
Updated•15 years ago
|
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Updated•15 years ago
|
blocking2.0: --- → ?
Comment 8•15 years ago
|
||
Hopefully we get someone to work on this soonish, but I don't think it'll be in the 1.9.3 timeframe.
blocking2.0: ? → -
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #8) > Hopefully we get someone to work on this soonish, but I don't think it'll be in > the 1.9.3 timeframe. Oh yeah? ;-) Added a patch. Flagging jeff for review.
Attachment #453839 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → bobbyholley+bmo
Reporter | ||
Comment 10•14 years ago
|
||
It would be nice to get a summary of what's changing here. In the new version it looks like the timer ticks even when there is no work to do, but in the old version it didn't?
Assignee | ||
Comment 11•14 years ago
|
||
Here's a summary of what's going on with this patch: In the status quo, when an image wants to put itself on the chopping block for discarding, it makes a new ONE_SHOT nsITimer for itself, and continually resets that timer any time it's touched (for example, when it's drawn). If an image changes state such that discarding is no longer possible, the timer is canceled. Conversely, if the image changes state such that discarding becomes possible, the timer is set. When the timer fires, it activates a callback function on the imgContainer that discards the backing frames. This is problematic because it turns out that timers can involve various system calls and can be really, really expensive. So we're willing to give up precision in the interval between "last touch" and "discard" if it means having one timer instead of dozens. This patch makes a new static tracker class called imgDiscardTracker to handle this. When an image wants to put itself on the chopping block, or when it's touched, it calls imgDiscardTracker::Reset() (functionally equivalent to the old ResetDiscardTimer() method). When an image wants to remove itself from the chopping block, it calls imgDiscardTracker::Remove(). The discard tracker maintains a repeating timer, a linked list of imgContainers, as well as a sentinel node. When an image calls Reset(), it is moved to the back of the queue. When the timer fires, it calls Discard() on everything in front of it in the queue, and then moves itself to the back of the queue. So to avoid being discarded, an image must call Reset() within DISCARD_TIMEOUT milliseconds after the moment when the sentinel lands behind it. Think of it as a trial period for images to prove their worth: when the sentinel is moved behind a bunch of images at the end of one timer callback, those images have until the next callback to do something interesting or else they're doomed to discardom. If, upon moving itself to the back, the sentinel still finds nothing in front of it, then there's nobody on trial, and so there isn't any point to running the timer. In this case we disable the timer, and switch it back on as soon as the queue has a non-trivial resident.
Reporter | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > Here's a summary of what's going on with this patch: This sounds very nice.
Reporter | ||
Comment 13•14 years ago
|
||
Comment on attachment 453839 [details] [diff] [review] patch v1 >diff --git a/modules/libpr0n/src/imgDiscardTracker.cpp b/modules/libpr0n/src/imgDiscardTracker.cpp >new file mode 100644 >--- /dev/null >+++ b/modules/libpr0n/src/imgDiscardTracker.cpp >@@ -0,0 +1,209 @@ >+nsresult >+imgDiscardTracker::Reset(imgDiscardTrackerNode *node) >+{ >+ nsresult rv; >+ PRBool isSentinel = (node == &sSentinel); >+ >+ // Sanity check the node. >+ NS_ABORT_IF_FALSE(isSentinel || node->curr, "Node doesn't point to anything!"); >+ >+ // We should not call this function if we can't discard >+ NS_ABORT_IF_FALSE(isSentinel || node->curr->CanDiscard(), >+ "trying to reset discarding but can't discard!"); >+ >+ // As soon as an image becomes animated it is set non-discardable >+ NS_ABORT_IF_FALSE(isSentinel || !node->curr->mAnim, >+ "Trying to reset discarding on animated image!"); We seem to be checking isSentinel three times here? >+/** >+ * Initialize the tracker. >+ */ >+nsresult >+imgDiscardTracker::Initialize() >+{ >+ nsresult rv; >+ >+ // Set up the list. Head<->Sentinel<->Tail >+ sHead.curr = sTail.curr = sSentinel.curr = nsnull; >+ sHead.prev = sTail.next = nsnull; >+ sHead.next = sTail.prev = &sSentinel; >+ sSentinel.prev = &sHead; >+ sSentinel.next = &sTail; >+ >+ // Create and start the timer >+ nsCOMPtr<nsITimer> t = do_CreateInstance("@mozilla.org/timer;1"); >+ NS_ENSURE_TRUE(t, NS_ERROR_OUT_OF_MEMORY); The general feeling is that these ENSURE type macros are bad because they hide control flow. I'd prefer if you open coded them. >+/* >+ * Disables the timer. No-op if the timer isn't running. >+ */ >+nsresult >+imgDiscardTracker::TimerOff() >+{ >+ // Nothing to do if the timer's already off. >+ if (!sTimerOn) >+ return NS_OK; >+ sTimerOn = PR_FALSE; >+ >+ // Deactivate >+ return sTimer->Cancel(); Cancel() seems to always return NS_OK so I wouldn't complain if this function returned void >+} >+ >+/** >+ * Timer callback. >+ */ Not really a helpful comment. There are a couple of functions that have similar comments that really just restate their names, I don't think there needed. >diff --git a/modules/libpr0n/src/imgDiscardTracker.h b/modules/libpr0n/src/imgDiscardTracker.h >+ >+#ifndef __imgDiscardTracker_h__ >+#define __imgDiscardTracker_h__ >+ >+class imgContainer; >+class nsITimer; >+ >+struct imgDiscardTrackerNode >+{ >+ imgContainer *curr; Might be worth adding a comment saying that this is a weak reference that is removed when the imgContainer is deleted.
Attachment #453839 -
Flags: review?(jmuizelaar) → review-
Reporter | ||
Comment 14•14 years ago
|
||
Comment on attachment 453839 [details] [diff] [review] patch v1 Mostly looks good though.
Assignee | ||
Comment 15•14 years ago
|
||
Updated patch addressing jeff's comments. Reflagging for review.
Attachment #453839 -
Attachment is obsolete: true
Attachment #455247 -
Flags: review?(jmuizelaar)
Reporter | ||
Updated•14 years ago
|
Attachment #455247 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 16•14 years ago
|
||
pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/df154e9bd999 Resolving fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•