Last Comment Bug 776200 - source compression kills SunSpider speed
: source compression kills SunSpider speed
Status: RESOLVED FIXED
[js:p1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Benjamin Peterson
:
:
Mentors:
Depends on:
Blocks: 776233 776741
  Show dependency treegraph
 
Reported: 2012-07-20 19:47 PDT by :Benjamin Peterson
Modified: 2012-07-26 07:06 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
hack (754 bytes, patch)
2012-07-20 19:47 PDT, :Benjamin Peterson
nicolas.b.pierron: review+
Details | Diff | Splinter Review
reduce compression level (1.58 KB, patch)
2012-07-20 20:21 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
don't compress files with huge strings (16.79 KB, patch)
2012-07-23 16:50 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
don't do anything (1.06 KB, patch)
2012-07-24 11:02 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-07-20 19:47:19 PDT
Created attachment 644573 [details] [diff] [review]
hack

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.
Comment 1 Nicolas B. Pierron [:nbp] 2012-07-20 19:55:42 PDT
Comment on attachment 644573 [details] [diff] [review]
hack

Review of attachment 644573 [details] [diff] [review]:
-----------------------------------------------------------------

Doc: http://www.zlib.net/manual.html#Constants
Comment 2 :Benjamin Peterson 2012-07-20 20:21:48 PDT
Created attachment 644583 [details] [diff] [review]
reduce compression level
Comment 3 :Benjamin Peterson 2012-07-20 23:54:55 PDT
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.
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-07-22 12:44:11 PDT
https://hg.mozilla.org/mozilla-central/rev/86a52b537fb6
Comment 6 Manoj 2012-07-23 07:32:51 PDT
(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?
Comment 7 :Benjamin Peterson 2012-07-23 08:56:06 PDT
(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.
Comment 8 :Benjamin Peterson 2012-07-23 16:50:13 PDT
Created attachment 645117 [details] [diff] [review]
don't compress files with huge strings

This implements the heuristic suggested by Luke. It halts compression if it finds huge strings.
Comment 9 Luke Wagner [:luke] 2012-07-23 18:32:09 PDT
Does this get back the remaining ms on regexp-dna?
Comment 10 :Benjamin Peterson 2012-07-23 18:43:33 PDT
Yes.
Comment 11 David Mandelin [:dmandelin] 2012-07-24 10:49:31 PDT
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.
Comment 12 :Benjamin Peterson 2012-07-24 11:02:27 PDT
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?
Comment 13 David Mandelin [:dmandelin] 2012-07-24 12:07:37 PDT
(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 David Mandelin [:dmandelin] 2012-07-24 13:02:44 PDT
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 Paul Wright 2012-07-24 13:25:39 PDT
Perhaps this is a good example of the need for a "no regression without prior mgt approval" policy?
Comment 16 David Mandelin [:dmandelin] 2012-07-24 17:14:50 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.