776200 - source compression kills SunSpider speed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

Attachment: 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]]

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]

Attachment: reduce compression level

Comment 3

[:Benjamin Peterson]

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 4

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86a52b537fb6

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86a52b537fb6

Comment 6

[Manoj]

(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]

(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]

Attachment: 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]]

Does this get back the remaining ms on regexp-dna?

Comment 10

[:Benjamin Peterson]

Yes.

Comment 11

[David Mandelin [:dmandelin]]

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]

Attachment: 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]]

(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 <[e-mail]>
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 <[e-mail]>
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]]

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]

Perhaps this is a good example of the need for a "no regression without prior mgt approval" policy?

Comment 16

[David Mandelin [:dmandelin]]

(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.