Closed
Bug 776200
Opened 12 years ago
Closed 12 years ago
source compression kills SunSpider speed
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Benjamin, Assigned: Benjamin)
References
Details
(Whiteboard: [js:p1])
Attachments
(2 files, 2 obsolete files)
16.79 KB,
patch
|
Details | Diff | Splinter Review | |
1.06 KB,
patch
|
Details | Diff | Splinter Review |
Bug 761723 regressed sunspider speed. The worst offender is regex-dna, which is basically a giant string. What seems to happen is compilation finishes and blocks on source compression finishing. I'm attaching a hack, which recovers most of the speed. Probably, source compression will just have to be able to last longer. Another possibility is compilation could cancel compression if it ends before compression is finished.
Assignee | ||
Updated•12 years ago
|
Attachment #644573 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•12 years ago
|
Attachment #644573 -
Attachment is patch: true
Comment 1•12 years ago
|
||
Comment on attachment 644573 [details] [diff] [review] hack Review of attachment 644573 [details] [diff] [review]: ----------------------------------------------------------------- Doc: http://www.zlib.net/manual.html#Constants
Attachment #644573 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #644573 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
I'm starting to wonder if something is wrong with the threading. If I force compression to be on the main thread, sunspider's time is exactly the same as with compression off the main thread. I tried fiddling with nspr thread parameters, but nothing seemed to make a difference.
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a52b537fb6
Whiteboard: [leave-open]
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86a52b537fb6
(In reply to Benjamin Peterson from comment #4) > https://hg.mozilla.org/integration/mozilla-inbound/rev/86a52b537fb6 Did you intend to leave the printf in the checkin?
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Manoj from comment #6) > (In reply to Benjamin Peterson from comment #4) > > https://hg.mozilla.org/integration/mozilla-inbound/rev/86a52b537fb6 > > Did you intend to leave the printf in the checkin? No, that was debugging junk.
Updated•12 years ago
|
Blocks: 776233
status-firefox17:
--- → affected
tracking-firefox17:
--- → +
Whiteboard: [leave-open] → [js:p1][leave-open]
Assignee | ||
Comment 8•12 years ago
|
||
This implements the heuristic suggested by Luke. It halts compression if it finds huge strings.
Assignee: general → bpeterson
Attachment #644583 -
Attachment is obsolete: true
Attachment #645117 -
Flags: review?(luke)
Comment 9•12 years ago
|
||
Does this get back the remaining ms on regexp-dna?
Assignee | ||
Updated•12 years ago
|
Attachment #645117 -
Flags: review?(luke) → review?(jorendorff)
Assignee | ||
Comment 10•12 years ago
|
||
Yes.
Comment 11•12 years ago
|
||
Have you found out what happened to the other 3ms of performance? Note, please don't land anything before clearing that up--I don't want to get stuck with a lost perf hit we can't track down.
Assignee | ||
Comment 12•12 years ago
|
||
I can't really reproduce SunSpider with enough precision to measure 3ms changes on my system. Does this patch make a difference?
Comment 13•12 years ago
|
||
(In reply to Benjamin Peterson from comment #12) > Created attachment 645383 [details] [diff] [review] > don't do anything > > I can't really reproduce SunSpider with enough precision to measure 3ms > changes on my system. Does this patch make a difference? I found the 3 ms to be between: changeset: 99940:e080642175e6 user: Benjamin Peterson <benjamin@python.org> date: Fri Jul 20 20:17:38 2012 +0200 summary: Bug 761723 - Save script sources to implement Function.prototype.toString. r=jorendorff,njn,jimb,jst,Ms2ger and changeset: 99943:1abd39543f58 user: Benjamin Peterson <benjamin@python.org> date: Fri Jul 20 20:19:17 2012 +0200 summary: Bug 761723 - Add a runtime hook to retrieve source that wasn't saved. r=luke I wasn't able to localize it to a specific changeset in that range--it seems to be spread out, about 1ms per patch. The patch in comment 12 applied to the latter changeset reduces the total regression to less than 0.2ms. But applied to tip, I still seem to get the same 3ms total hit. So the 3ms might be something else. I'll keep looking.
Comment 14•12 years ago
|
||
I tried applying the backouts for the other 2 regressions that were in-tree when this landed to the changeset before it landed. That still seems to give a 3 ms regression, so the 3ms regression might come from somewhere else at this point. I saw what may have been a couple of 2ms regressions in range of 99461-99510, so I'll have to look there later this afternoon.
Comment 15•12 years ago
|
||
Perhaps this is a good example of the need for a "no regression without prior mgt approval" policy?
Comment 16•12 years ago
|
||
(In reply to Paul Wright from comment #15) > Perhaps this is a good example of the need for a "no regression without > prior mgt approval" policy? That is roughly how it works, modulo snafus; in this case the regression was inadvertent. We do need to back those out more quickly in the future, though. My bad, being final backstop and all. This is now fixed; I tracked down the missing 3ms and it's bug 777174.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [js:p1][leave-open] → [js:p1]
Assignee | ||
Updated•12 years ago
|
Attachment #645117 -
Flags: review?(jorendorff)
Updated•12 years ago
|
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•