The default bug view has changed. See this FAQ.

Fix eval cache performance regression

RESOLVED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ fixed)

Details

(Whiteboard: [js:p1])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 644256 [details] [diff] [review]
Patch

There's a pretty big SS date-format-tofte regression due to not adding scripts to the eval cache (regression from d6531ef05a6f).
Attachment #644256 - Flags: review?(bhackett1024)
(Assignee)

Comment 1

5 years ago
Need to land this on aurora too.
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
(Assignee)

Comment 2

5 years ago
Comment on attachment 644256 [details] [diff] [review]
Patch

Just remembered Brian is gone for the weekend so changing reviewer.
Attachment #644256 - Flags: review?(bhackett1024) → review?(dvander)

Comment 3

5 years ago
I see why d6531ef05a6f needed to not persist the EvalCacheLookup.  However, I don't see why the AddPtr p_ had to go.  Given that relookupOrAdd (using the AddPtr) avoids hashing the whole script again, it seems beneficial.  (On that subject Jan, does your attached patch recover all the slowdown?)

Unfortunately, there isn't a bug to refer to, Bill do you remember the hazard?
Attachment #644256 - Flags: review+
(Assignee)

Comment 4

5 years ago
(In reply to Luke Wagner [:luke] from comment #3)
> (On that subject Jan, does your attached patch recover all the slowdown?)

I got 15 ms on SS date-format-tofte before revision d6531ef05a6f and at tip with the patch. 15 ms is not much so maybe a micro-benchmark will show a difference. Okay if I land the attached patch to fix the regression and we file a follow-up bug to avoid hashing the script twice?
(Assignee)

Updated

5 years ago
Attachment #644256 - Flags: review?(dvander)

Comment 5

5 years ago
(In reply to Jan de Mooij (:jandem) from comment #4)
Of course, land away.  I was more just asking to understand if there was a real reason.

Comment 6

5 years ago
Oh, one request: can you turn the lookupForAdd+add into put?
Whiteboard: [js:p1]
tracking-firefox16: ? → +
tracking-firefox17: ? → +
Blocks: 776233
Fixed by backout of changeset d6531ef05a6f.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
(In reply to David Mandelin [:dmandelin] from comment #7)
> Fixed by backout of changeset d6531ef05a6f.
What about Aurora 16?
status-firefox17: affected → fixed
Target Milestone: --- → mozilla17
Also backed out of 16.

http://hg.mozilla.org/releases/mozilla-aurora/rev/151896de0ae9
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.