Closed Bug 614137 Opened 9 years ago Closed 9 years ago

cache eval CSP check

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

eval("") is slow for us in the browser because we perform a CSP check every time. According to jst JSP is static per page, so after the first check we can cache the result until we navigate (clear scope). This should be a hefty speedup for SS in the browser, hopefully (to be verified shortly).
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
I should put this into the global object instead.
Attached patch patch (obsolete) — Splinter Review
Stick the cache into the global to avoid confusion when contexts cross globals.
Attachment #492528 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #492533 - Attachment is obsolete: true
This also speeds up new Function(""), by the way (same security check in that path as well).
Attached patch patchSplinter Review
Attachment #492534 - Attachment is obsolete: true
This is a 50% speedup for date-format-xparb on my machine. I don't see a clear speedup for the other eval-using SS test cases. dmandelin will measure with some more detail tomorrow. Due to the magnitude of the speedup, I will ask for review and then approval to get this in.
Attachment #492539 - Flags: review?(mrbkap)
Attachment #492539 - Flags: approval2.0?
bz, if dmandelin can confirm this speedup, maybe we should spend a little time profiling SS and v8 in the browser. It seems there is still headroom in non-shell embeddings. Maybe work week?
Sounds fine to me, yeah.

That said, a good start is getting those extant GC issues sorted out and then just comparing runtimes in shell and browser to see what sort of headroom we can expect...
> Sounds fine to me, yeah.
> 
> That said, a good start is getting those extant GC issues sorted out and then
> just comparing runtimes in shell and browser to see what sort of headroom we
> can expect...

I don't expect you would find much more for SS. There could be a few ms, but I was comparing shell scores to browser scores a while back and the only slowdowns I saw were this and GC.(In reply to comment #9)
Yeah, I am all over the GC stuff.
Actually, looks like v8 is full of evals, too. We should measure that, too.
I don't get a speedup on my extracted date-format-tofte with this patch on Windows. Not sure why. Maybe it needs more detailed profiling in browser.
Comment on attachment 492539 [details] [diff] [review]
patch

I talked to dveditz and bsterne and they said that we might eventually allow extensions/mashups to change the CSP after page load, but for now they can't and we'll try to hold a strong line on the "inline scripts/eval" policies and say that that *can't* change after page load. So this looks good to me.
Attachment #492539 - Flags: review?(mrbkap) → review+
...and if they ever can change, it shouldn't be hard to make eval-CSP-checking use a "push" model rather than a "pull" model, I'd think.
Is this going to make Fx4?
Waiting for approval, still. Drivers?
What's the risk of regressions? If it regresses, would we find out quickly? I assume we can easily back it out if so?
Patch is pretty straight forward and is easy to back out. Its a security check and its covered by a test.
Comment on attachment 492539 [details] [diff] [review]
patch

OK, we did really want this, let's do it.
Attachment #492539 - Flags: approval2.0? → approval2.0+
Is somebody gonna check this in?
http://hg.mozilla.org/tracemonkey/rev/a4b72ddb9e6e
Whiteboard: fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.