Closed
Bug 425941
Opened 17 years ago
Closed 17 years ago
7% Tp regression on windows [fast qm-pxp-fast01]
Categories
(Core :: General, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: stevee, Assigned: joe)
References
Details
(Keywords: perf, regression)
Attachments
(3 files, 1 obsolete file)
37.07 KB,
image/png
|
Details | |
1.09 KB,
patch
|
Details | Diff | Splinter Review | |
2.97 KB,
patch
|
Details | Diff | Splinter Review |
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?
Comment 1•17 years ago
|
||
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•17 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
Updated•17 years ago
|
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → general
Updated•17 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 3•17 years ago
|
||
Was there a similar Tp jump when e.g. PNG and JPEG unloading landed?
Comment 4•17 years ago
|
||
no
Comment 5•17 years ago
|
||
(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•17 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•17 years ago
|
||
Assignee: nobody → joe
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•17 years ago
|
||
Assignee | ||
Comment 9•17 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.
Updated•17 years ago
|
Attachment #312925 -
Attachment description: patch do siable JPEG discarding → patch do disable JPEG discarding
Updated•17 years ago
|
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
Comment 11•17 years ago
|
||
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•17 years ago
|
||
(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•17 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).
Comment 14•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
||
No more work required here. Remaining discussion in bug 399925.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•