Closed
Bug 480621
Opened 16 years ago
Closed 14 years ago
Windows 3d-morph regression
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: sayrer, Assigned: jorendorff)
Details
Attachments
(1 file)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
data from qm-pxp-trunk-04
Mon Feb 23 20:57:00 2009 62ms
Tue Feb 24 08:07:00 2009 89ms
http://hg.mozilla.org/tracemonkey/pushloghtml?startdate=2009-02-23+20%3A00%3A00&enddate=2009-02-24+09%3A00%3A00
Reporter | ||
Comment 1•16 years ago
|
||
Here's a pic. Sorry about the X-Axis, my chart stuff is a little busted atm. I checked the raw data to find the regression range.
<http://chart.apis.google.com/chart?cht=lc&chs=400x300&chd=e:t2tgtLtgt2tgt2t2tgtgt2uMuMuMt2uMuMuMuMt2uMt2uMt2uMt2t2uMt2tLtgtLtgtgtgtLtgtLtLt2qxtgqbtLtLtLs1qxqGqGqGqbs11Cs1qxs1s1qGqxqbs1s1qxqbqbqbs1qxqbqxrHqxt2rztgrHsfqGqbqbqGtLrztLs1s1qxsfsfqGs1qGs1s1s1s1s1t2s1sfs1s1s1s1rHsfs1sfpwsfsfs1sfsftLs1qbsfyp8ks1s1s1uMsfs1tLs1sfyTs1VkVkVOUMVOVkVOVOVOVkVkVOVkVkVkVkVOVkVOVOVOVOU4U4VOVOVOVOU4VOVOU4U4VOVOVOVOVOVOVOVOVOVOVOedeHededeH&chco=FF0000&chf=c,ls,0,CCCCCC,0.2,FFFFFF,0.2&chxt=y&chxl=0:||50|100|150&chg=0,25,1,0>
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.1+
Priority: -- → P2
Assignee | ||
Comment 2•16 years ago
|
||
I tested this using MSVC7.1 (it's what I've got) and got results that are definitely not as damning as sayrer's:
25394:4d2d2fca8c0d : 250 234 234 234 234 234 250
25398:42acafcdae02 : 250 235 250 235 234 250 250
Inconclusive, I think. Something different must be happening in MSVC8.
If anyone else is willing to take this, I'd be grateful. If not, I need to get a Windows dev machine.
Comment 3•16 years ago
|
||
I was going to try this out today. I have MSVC8 installed.
Comment 4•16 years ago
|
||
Are we even running the same versions and OS and everything? My results are so different I can hardly believe I'm running the right test. For me:
25394:4d2d2fca8c0d: 25ms
25398:42acafcdae02: 24ms
This is MSVC8/Vista/Core 2 Duo E7300 2.67 GHz.
Reporter | ||
Comment 5•16 years ago
|
||
This is still showing a regression.
Comment 6•16 years ago
|
||
I still can't duplicate the regression. I tried 25394:4d2d2fca8c0d vs tip and got about 42 ms in both cases.
Comment 7•16 years ago
|
||
Vista, core 2 duo, vc8 toolchain here, likewise can't reproduce. Even cranking the iteration count way up to get a more stable multi-second measure, the range in question shows identical times at both ends, and tip is a solid 10-15% faster than either.
Comment 8•16 years ago
|
||
sayrer, do you still see a regression here?
Reporter | ||
Comment 9•16 years ago
|
||
still there. it's not me seeing it, it's the tinderbox.
Comment 10•16 years ago
|
||
OK. So:
- What is the tinderbox actually running? Is this Tsspider, or what?
- What does the line on the graph mean?
- What kind of machine/OS/etc is this tinderbox?
Comment 11•15 years ago
|
||
I have finally managed to duplicate this. It seems to have occurred on this revision:
changeset: 25488:42acafcdae02
user: Jason Orendorff <jorendorff@mozilla.com>
date: Mon Feb 23 17:31:02 2009 -0600
summary: Bug 477279 - Tune dense array growth. r=brendan.
It happens only with --enable-jemalloc. Without jemalloc, 3d-morph perf is the same on this revision and the previous.
Comment 12•15 years ago
|
||
graydon mentioned that for certain large memory sizes jemalloc hits mmap and munmap for every allocation.
Comment 13•15 years ago
|
||
In 3d-morph, there seems to be one array that gets created and resized. The capacity goes up to 65535 in this test. Given that there are only 14 calls to ResizeSlots made from EnsureCapacity (in the new version), it's hard to see how they could take a full 14 ms longer than previously. I was wondering if having the array bigger than needed could make GC take longer, but that doesn't make a lot of sense given that this happens only for jemalloc builds, and AFAICT, only in the browser.
Maybe this?
Comment 15•15 years ago
|
||
Comment on attachment 377049 [details] [diff] [review]
untested: limit dense array trace to largest possibly-set value
Looks good to me, hard to believe capacity outraces length so much that it would matter very often, but definitely worth doing regardless.
Attachment #377049 -
Flags: review+
Reporter | ||
Updated•15 years ago
|
Assignee: general → jorendorff
Assignee | ||
Comment 16•15 years ago
|
||
Even --enable-threadsafe --enable-jemalloc I can't reproduce this in the shell on Linux. I verified with objdump that js is actually linked with libjemalloc.a.
25394:4d2d2fca8c0d 123.8ms +/- 4.7%
25398:42acafcdae02 120.8ms +/- 2.8%
Assignee | ||
Comment 17•15 years ago
|
||
Sorry, wrong numbers.
25394:4d2d2fca8c0d 50.9ms +/- 5.2%
25398:42acafcdae02 51.4ms +/- 6.2%
Comment 18•15 years ago
|
||
(In reply to comment #16)
> Even --enable-threadsafe --enable-jemalloc I can't reproduce this in the shell
> on Linux. I verified with objdump that js is actually linked with
> libjemalloc.a.
I have only been able to duplicate this with all of these selections:
- Windows
- --enable-jemalloc
- browser Talos tsspider test
Windows may not be required; I didn't test the same way on other platforms. But I found that on Windows I could not duplicate without using jemalloc. And no other test harness but browser Talos could duplicate it.
Comment 19•15 years ago
|
||
Guys, how are we doing on this? Do we have enough information to tell where exactly the problem exists? Also, who is actively working on this, Jason or dmandelin? Appreciate any update.
Comment 20•15 years ago
|
||
I'm working on it. Hopefully jorendorff is as well. So far I have determined that the gc patch doesn't help, that the first call site to EnsureCapacity (around line 457 or so) is the one to blame, and that it doesn't *seem* any extra time is being spent in ResizeSlots in the regressed version.
Reporter | ||
Updated•15 years ago
|
Flags: wanted1.9.1+
Flags: blocking1.9.2+
Flags: blocking1.9.1+
Reporter | ||
Updated•15 years ago
|
Flags: blocking1.9.2+ → blocking1.9.2-
Comment 21•14 years ago
|
||
These bugs are all part of a search I made for js bugs that are getting lost in transit:
http://tinyurl.com/jsDeadEndBugs
They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.
We've long surpassed that perf.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•