425941 - 7% Tp regression on windows [fast qm-pxp-fast01]

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Steve England [:stevee]]

Attachment: image showing graph

Tp has jumped from around 209ms to 224ms

WINNT 5.1 mini talos trunk fast qm-pxp-fast01

build start: 2008/03/28 01:28:02 : Tp=209.55
build start: 2008/03/28 03:04:46 : Tp=224.88

Checkins to module PhoenixTinderbox between 2008-03-28 01:28 and 2008-03-28 03:04 :
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-28+01%3A28&maxdate=2008-03-28+03%3A04&cvsroot=%2Fcvsroot

Graph:
http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1206799611&bpst=cursor&bpstart=0&bpend=1206799611&m1tid=148138&m1bl=0&m1avg=0

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I think the talos "build start" dates are actually "build retrieved from ftp server" dates, so you'd need to adjust accordingly?

CCing some likely candidates from looking at a bit broader of a range.

Comment 2

[Steve England [:stevee]]

Reed backed out bug 399925 for a test and the Tp value fell from ~223ms to ~208ms, so we have a cause.

Comment 3

[Joe Drew (not getting mail)]

Was there a similar Tp jump when e.g. PNG and JPEG unloading landed?

Comment 4

[Stuart Parmenter]

no

Comment 5

[Reed Loden [:reed]]

(In reply to comment #3)
> Was there a similar Tp jump when e.g. PNG and JPEG unloading landed?

There was definitely a Tp jump/regression when PNG and JPEG discarding landed (see bug 296818, comment #41 and #42), but I'm not sure if it was of this magnitude... Worth looking into, though.

Comment 6

[Alfred Kayser]

Note that for GIF images, most of them are probably animated images (as that is one of the reasons to use GIF), or single pixel images (space.gif).
For these two kinds the 'DiscardImageData' only brings overhead.
So, we could try to only enable the GIF image discarding for non-animated/non-single pixel images, and see what that does to the Tp.

Comment 7

[Joe Drew (not getting mail)]

Attachment: patch to disable JPEG discarding

Comment 8

[Joe Drew (not getting mail)]

Attachment: patch to disable PNG discarding

Comment 9

[Joe Drew (not getting mail)]

Those two patches disable discarding of decoded data for JPEG and PNG images, respectively. My plan is to get them checked in to test the Tp improvement; at that time, we can decide what we want to do.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marking blocking to get results of investigation -- once we figure out whether it was caused by JPEG/PNG discarding, we should decide what to do.  I suspect we'll want the memory savings, but we'll see.

Comment 11

[Johnathan Nightingale [:johnath]]

Joe asked if we could close the tree for him to test-land these (or for me to, on his behalf).  I suggested he send a note to d.a.f and dev.planning first, giving people a heads-up that we will try this tomorrow morning EDT.  I'll land the JPEG patch around 6:30AM EDT, and then once the talos boxes are churning on that build, will backout the JPEG patch and land PNG, with an eye towards having the talos boxes pick up that build, and then re-opening the tree by 9AM PDT.

I have also brought ted up to speed, since he is tomorrow's sheriff.

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #10)
> once we figure out whether
> it was caused by JPEG/PNG discarding, we should decide what to do.  I suspect
> we'll want the memory savings, but we'll see.

Maybe look at discarding data only for images above a certain size? For example, a 32x32 image is only 4K decoded, and the source file probably isn't much smaller. It could even be bigger (pathological compression, EXIF data, color profile, etc). You'd even save on short-term memory usage (because you're you're not lugging around the source image).

Comment 13

[Joe Drew (not getting mail)]

If it's shown to be a general discarding-related Tp loss, I'll look into profiling to see what I can see. Unless we have real data, it's probably premature to talk specific optimization methods (although if there's no real smoking gun, Dolske's idea is probably a pretty good one).

Comment 14

[Johnathan Nightingale [:johnath]]

I have test-landed the JPEG patch at 06:03PDT

Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.93; previous revision: 1.92
done

Comment 15

[Johnathan Nightingale [:johnath]]

And now I've backed it out - the windows talos runs using the dep build from 07:05PDT are the ones with this patch.

Because the tree was orange (and burning!) when I started, and that took a while to resolve, there wasn't time to land the PNG patch. 

Checking in modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v  <--  nsJPEGDecoder.cpp
new revision: 1.94; previous revision: 1.93
done

Comment 16

[Joe Drew (not getting mail)]

Attachment: disable more unloading stuff for JPEG

Well, thanks to Johnathan I've discovered that just disabling the discarding callback doesn't save us any time on Tp at all. My only remaining theory is that the remaining discarding infrastructure (e.g., the saving of the undecoded data) is the only remaining culprit. This patch disables that for JPEG.

Comment 17

[Mike Schroepfer]

(In reply to comment #16)
> Created an attachment (id=313662) [details]
> disable more unloading stuff for JPEG
> 
> Well, thanks to Johnathan I've discovered that just disabling the discarding
> callback doesn't save us any time on Tp at all. My only remaining theory is
> that the remaining discarding infrastructure (e.g., the saving of the undecoded
> data) is the only remaining culprit. This patch disables that for JPEG.

Should we get this reviewed/landed then?

Comment 18

[Joe Drew (not getting mail)]

No, it won't do any good. I can reproduce the Tp regression on my local machine with local talos and the gif discarding patch, but there is no Tp gain when I apply that patch to disable JPEG discarding.

That leaves me with optimizing the GIF discarding patch, which I'm working on.

Comment 19

[Joe Drew (not getting mail)]

I've found nothing promising in my GIF discarding optimization attempts. But what's more disturbing is the following graph:

http://graphs.mozilla.org/graph.html#spst=range&spstart=1202931767&spend=1207633375&bpst=Cursor&bpstart=1202931767&bpend=1207633375&m1tid=148144&m1bl=0&m1avg=0&m2tid=148147&m2bl=0&m2avg=0

Note the very small bump upwards betwen 2008-03-28 03:16:48 and 2008-03-30 03:56:54. This corresponds exactly to the Tp jump caused by the patch for bug 399925, and I have to attribute it to that unless someone else can let me know that there's another patch landed and then backed out in the same timeframe. (Reed, any input?)

This is of course highly fishy for a patch that's supposed to save us memory. It could be that most gif files on the internet now decode to smaller than their raw data size, I suppose.

I'll look into this further tomorrow.

Comment 20

[Alfred Kayser]

It is probably true that most GIF's are nowaday either small (< 32x32) or are animating. Animations are never discarded (unless they are single loop?), so keeping the raw data for these is only overhead (and very costly at that). 
And as comment 12 says, keeping the raw data for small images doesn't provide much benefit. This also applies to PNG's (and a little to JPG's)
So, one or two bugs/patches to be addressed:
1. Don't keep raw data for small images (all types)
2. Don't keep raw data for animated images (all types)

Another things that one can do:
1. Optimize the way the raw data is stored (now one big reallocated buffer)
2. Optimize the way that images are restored from raw data (instead of going through a InputStream construct, push data directly into the 'ProcessData' of the image decoder.

Comment 21

[Joe Drew (not getting mail)]

Given the mem _increase_ from the gif discarding patch, as well as the Tp jump, I'm inclined to resolve this fixed and bug 399925 WONTFIX.

Comment 22

[Joe Drew (not getting mail)]

No more work required here. Remaining discussion in bug 399925.