Last Comment Bug 775915 - Fix eval cache performance regression
: Fix eval cache performance regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jan de Mooij [:jandem]
: general
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 776233
  Show dependency treegraph
Reported: 2012-07-20 04:44 PDT by Jan de Mooij [:jandem]
Modified: 2012-07-25 16:24 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1.61 KB, patch)
2012-07-20 04:44 PDT, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-07-20 04:44:36 PDT
Created attachment 644256 [details] [diff] [review]

There's a pretty big SS date-format-tofte regression due to not adding scripts to the eval cache (regression from d6531ef05a6f).
Comment 1 Jan de Mooij [:jandem] 2012-07-20 04:49:07 PDT
Need to land this on aurora too.
Comment 2 Jan de Mooij [:jandem] 2012-07-20 04:53:58 PDT
Comment on attachment 644256 [details] [diff] [review]

Just remembered Brian is gone for the weekend so changing reviewer.
Comment 3 Luke Wagner [:luke] 2012-07-20 10:14:25 PDT
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?
Comment 4 Jan de Mooij [:jandem] 2012-07-20 11:52:33 PDT
(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?
Comment 5 Luke Wagner [:luke] 2012-07-20 13:51:59 PDT
(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 Luke Wagner [:luke] 2012-07-20 13:52:43 PDT
Oh, one request: can you turn the lookupForAdd+add into put?
Comment 7 David Mandelin [:dmandelin] 2012-07-23 18:28:41 PDT
Fixed by backout of changeset d6531ef05a6f.
Comment 8 Scoobidiver (away) 2012-07-24 00:11:39 PDT
(In reply to David Mandelin [:dmandelin] from comment #7)
> Fixed by backout of changeset d6531ef05a6f.
What about Aurora 16?
Comment 9 David Mandelin [:dmandelin] 2012-07-25 16:24:05 PDT
Also backed out of 16.

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