Jeff Muizelaar [:jrmuizel] Reporter
	Description • 15 years ago

      No description provided.

	Bobby Holley (:bholley) 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?

	Joe Drew (not getting mail)
	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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 4 • 15 years ago

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.

	Bobby Holley (:bholley) Assignee
	Comment 5 • 15 years ago

on my post bug 435296 todo list. I'll get to it mid-to-late august.

	Bobby Holley (:bholley) 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.

	Bobby Holley (:bholley) 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).

	Joe Drew (not getting mail)
	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.

	Bobby Holley (:bholley) 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.

	Jeff Muizelaar [:jrmuizel] 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?

	Bobby Holley (:bholley) 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.

	Jeff Muizelaar [:jrmuizel] 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.

	Jeff Muizelaar [:jrmuizel] 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.

	Jeff Muizelaar [:jrmuizel] Reporter
	Comment 14 • 14 years ago

Comment on attachment 453839 [details] [diff] [review]
patch v1

Mostly looks good though.

	Bobby Holley (:bholley) Assignee
	Comment 15 • 14 years ago

Updated patch addressing jeff's comments. Reflagging for review.

	Bobby Holley (:bholley) Assignee
	Comment 16 • 14 years ago

pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/df154e9bd999

Resolving fixed.