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

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: stevee, Assigned: joe)

Tracking

({perf, regression})

Trunk
x86
Windows XP
perf, regression
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 312474 [details]
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
Flags: blocking-firefox3?
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.
(Reporter)

Comment 2

11 years ago
Reed backed out bug 399925 for a test and the Tp value fell from ~223ms to ~208ms, so we have a cause.
Blocks: 399925
Component: General → General
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → general
Flags: blocking1.9?
(Assignee)

Comment 3

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

Comment 4

11 years ago
no
(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

11 years ago
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.
(Assignee)

Comment 7

11 years ago
Created attachment 312925 [details] [diff] [review]
patch to disable JPEG discarding
Assignee: nobody → joe
Status: NEW → ASSIGNED
(Assignee)

Comment 8

11 years ago
Created attachment 312927 [details] [diff] [review]
patch to disable PNG discarding
(Assignee)

Comment 9

11 years ago
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.
Attachment #312925 - Attachment description: patch do siable JPEG discarding → patch do disable JPEG discarding
Attachment #312925 - Attachment description: patch do disable JPEG discarding → patch to disable JPEG discarding
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.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
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.
(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).
(Assignee)

Comment 13

11 years ago
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).
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
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
(Assignee)

Comment 16

11 years ago
Created attachment 313662 [details] [diff] [review]
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.
Attachment #312925 - Attachment is obsolete: true

Comment 17

11 years ago
(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?
(Assignee)

Comment 18

11 years ago
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.
(Assignee)

Comment 19

11 years ago
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

11 years ago
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.
(Assignee)

Comment 21

11 years ago
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.
(Assignee)

Comment 22

11 years ago
No more work required here. Remaining discussion in bug 399925.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.