source compression kills SunSpider speed

RESOLVED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [js:p1])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #644573 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

5 years ago
Attachment #644573 - Attachment is patch: true
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

5 years ago
Created attachment 644583 [details] [diff] [review]
reduce compression level
Attachment #644573 - Attachment is obsolete: true
(Assignee)

Comment 3

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a52b537fb6
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/86a52b537fb6

Comment 6

5 years ago
(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

5 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.
Blocks: 776233
status-firefox17: --- → affected
tracking-firefox17: --- → +
Whiteboard: [leave-open] → [js:p1][leave-open]
Blocks: 776741
(Assignee)

Comment 8

5 years ago
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.
Assignee: general → bpeterson
Attachment #644583 - Attachment is obsolete: true
Attachment #645117 - Flags: review?(luke)

Comment 9

5 years ago
Does this get back the remaining ms on regexp-dna?
(Assignee)

Updated

5 years ago
Attachment #645117 - Flags: review?(luke) → review?(jorendorff)
(Assignee)

Comment 10

5 years ago
Yes.
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

5 years ago
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?
(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.
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

5 years ago
Perhaps this is a good example of the need for a "no regression without prior mgt approval" policy?
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [js:p1][leave-open] → [js:p1]
(Assignee)

Updated

5 years ago
Attachment #645117 - Flags: review?(jorendorff)

Updated

5 years ago
status-firefox17: affected → fixed
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.