583028 - Tp4 regression from async imagelib notification

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

There was a pretty large (1-5%) regression from the async imagelib patch. We should work out whether this happened on only some pages and what caused it.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Here's a detailed graph of what happened:
http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs&old=4894012&new=4896393

Comment 2

[Joe Drew (not getting mail)]

http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs&old=4894012&new=4896393 is, I believe, an accurate representation of what happened before and after. (Tp4 doesn't seem to get run on all checkins on OS X, for some reason, so this doesn't correspond precisely to before and after my checkin, but it's close enough.) 

So, mostly what I see there is that there's an overall loss of performance, with one or two sites taking a lot longer. I'll investigate this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, I'd probably start with livescore.com as something that might be particularly amenable to profiling+debugging.

Comment 4

[Joe Drew (not getting mail)]

We spend a ton of time just dispatching events. I'm going to try coalescing events ("do we have an event pending? add this proxy to a list to be notified by it") rather than sending a new one every time.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Huh.  How many events are we talking here for a typical web page?

Comment 6

[Joe Drew (not getting mail)]

One per duplicated image. The livescore.com pages have quite a number of duplicated images.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is "quite a number" dozens, or thousands?

Comment 8

[Joe Drew (not getting mail)]

Hundreds.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  It still feels like tens-of-milliseconds processing times for that is a lot, but maybe that's happening....

Comment 10

[Bobby Holley (:bholley)]

(In reply to comment #9)
> OK.  It still feels like tens-of-milliseconds processing times for that is a
> lot, but maybe that's happening....

Maybe this is a good profile case for bug 583069?

Comment 11

[Joe Drew (not getting mail)]

I should note that all the time was _under_ SyncNotify. Also, I just reprofiled on livescore.com, this time with an internet connection (it turns out that livescore.com, even in our Tp4 page set, contains a reference to a live web page), and SyncNotify and children was down to 1.3% of time on a constantly-refreshing copy of livescore.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Maybe this is a good profile case for bug 583069?

That can't explain the _Linux_ regression.

> I should note that all the time was _under_ SyncNotify.

OK, then reducing the number of runnables is not likely to help... where was the time actually spent?

Comment 13

[Joe Drew (not getting mail)]

In handling OnStopDecode - and whatever is implied by that, including a bunch of JS stuff. All standard.

I have great faith in my plan to mitigate this onload slowdown by not queueing extra tasks when there's already one queued. That way, we won't end up at the end of the event queue, making onload fire much later than it would have otherwise.

Comment 14

[Joe Drew (not getting mail)]

Attachment: delay onload less

Instead of always firing off new runnables, we check to see if a runnable already exists and append a proxy to its list of proxies to notify.

Note that I didn't implement this for NotifyCurrentState because a) it's called a lot less frequently (only for images that come from print preview) and b) it would be very ugly having both a copy of the imgStatusTracker to keep the status you want to update, and a reference to a different imgStatusTracker to update when you actually get run.

This is currently running through try server.

Comment 15

[Bobby Holley (:bholley)]

(In reply to comment #14)
> 
> This is currently running through try server.

Let me know when you've got confirmation that this fixes the regression.

Comment 16

[Joe Drew (not getting mail)]

Won't be until tomorrow at the earliest; all trees, including try, are all kinds of broken right now.

Comment 17

[Joe Drew (not getting mail)]

http://people.mozilla.org/~jmuizelaar/graph-server/detail.html?host=graphs&old=4896393&new=4976045

Results look very good - big green bars for livescore.com, for example. There are a couple of red bars, but there's also a fair amount of noise in these tests, from comparing different mozilla-central runs.

Comment 18

[Joe Drew (not getting mail)]

Attachment: delay onload less, and compile in debug mode

nsCOMPtr can't hold on to a forward-declared class when we're compiling debug; work around that.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 463655 [details] [diff] [review]
delay onload less, and compile in debug mode

diff --git a/modules/libpr0n/src/imgStatusTracker.cpp b/modules/libpr0n/src/imgStatusTracker.cpp
--- a/modules/libpr0n/src/imgStatusTracker.cpp
+++ b/modules/libpr0n/src/imgStatusTracker.cpp


+  imgRequestNotifyRunnable* runnable = static_cast<imgRequestNotifyRunnable*>(mRequestRunnable.get());
+  if (runnable && runnable->mRequest == request) {

Add a comment here explaining that this is to handle ChangeOwner(), and file a bug for the issue we talked about on IRC where calling ChangeOwner while a notification is pending causes consumers to be notified with the wrong state.


+  // We don't keep track of 

What?

diff --git a/modules/libpr0n/src/imgStatusTracker.h b/modules/libpr0n/src/imgStatusTracker.h
--- a/modules/libpr0n/src/imgStatusTracker.h
+++ b/modules/libpr0n/src/imgStatusTracker.h

   // alive as long as this instance is, because we hold a weak reference to it.
   imgStatusTracker(imgIContainer* aImage);
+  imgStatusTracker(const imgStatusTracker& aOther);

Please declare an explicit operator= here and flag it "not to be implemented" (see layout/style/nsStyleStruct.h) so that we don't accidentally use it.

This patch coalesces things so that we only ever have 1 event per imgRequest. I'd really prefer to coalesce things all the way with a tracker so that we only ever have 1 event per imagelib, but joe doesn't really want to do that right now. r=bholley with the above fixes. If it doesn't fix the regression, I think that's probably the next step.

Comment 20

[Bobby Holley (:bholley)]

For reference, the tryserver builds here are at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c133ae00e4a3/

Comment 21

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/c3d482704ba8

Comment 22

[Bobby Holley (:bholley)]

Let's leave this open until we confirm that the regression was fixed.

Comment 23

[Joe Drew (not getting mail)]

I do see a small downtick in Tp4 numbers, but it doesn't seem to have been large enough to be picked up by the regression finder script.

http://graphs.mozilla.org/#tests=[[38,1,530],[38,1,531],[38,1,532],[38,1,533],[38,1,534],[38,1,535],[38,1,536],[38,1,537],[38,1,538],[38,1,539],[38,1,540],[38,1,576],[38,1,577],[38,1,578],[38,1,579],[38,1,580],[38,1,581],[38,1,582],[38,1,718],[38,1,719],[38,1,720],[38,1,721],[38,1,722],[38,1,723],[38,1,724],[38,1,725],[38,1,726],[38,1,727],[38,1,728],[38,1,729],[38,1,730],[38,1,731],[38,1,732],[38,1,733],[38,1,734],[38,1,735],[38,1,736],[38,1,737],[38,1,738],[38,1,739],[38,1,740],[38,1,741],[38,1,742],[38,1,743],[38,1,744],[38,1,745],[38,1,746],[38,1,747],[38,1,900],[38,1,901],[38,1,902],[38,1,903],[38,1,904],[38,1,391],[38,1,392],[38,1,393],[38,1,394],[38,1,395],[38,1,396],[38,1,397],[38,1,398],[38,1,399],[38,1,400],[38,1,401],[38,1,402],[38,1,403],[38,1,404],[38,1,405],[38,1,406],[38,1,407],[38,1,408],[38,1,409],[38,1,410],[38,1,523],[38,1,524],[38,1,525],[38,1,526],[38,1,527],[38,1,528],[38,1,529],[38,1,541],[38,1,542],[38,1,543],[38,1,544],[38,1,545],[38,1,546],[38,1,547],[38,1,548],[38,1,549],[38,1,550],[38,1,551],[38,1,552],[38,1,553],[38,1,668],[38,1,669],[38,1,670],[38,1,671],[38,1,672],[38,1,673],[38,1,674],[38,1,675],[38,1,676],[38,1,677],[38,1,889],[38,1,890],[38,1,891]]&sel=1279886236,1281713079

Comment 24

[Joe Drew (not getting mail)]

This actually looks to be fixed now.