614137 - cache eval CSP check

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

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

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

I should put this into the global object instead.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Stick the cache into the global to avoid confusion when contexts cross globals.

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Andreas Gal :gal]

This also speeds up new Function(""), by the way (same security check in that path as well).

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

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.

Comment 8

[Andreas Gal :gal]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[David Mandelin [:dmandelin]]

> 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)

Comment 11

[Andreas Gal :gal]

Yeah, I am all over the GC stuff.

Comment 12

[Andreas Gal :gal]

Actually, looks like v8 is full of evals, too. We should measure that, too.

Comment 13

[David Mandelin [:dmandelin]]

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 14

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 15

[Jeff Walden [:Waldo]]

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

Comment 16

[Harsh86]

Is this going to make Fx4?

Comment 17

[Andreas Gal :gal]

Waiting for approval, still. Drivers?

Comment 18

[David Mandelin [:dmandelin]]

What's the risk of regressions? If it regresses, would we find out quickly? I assume we can easily back it out if so?

Comment 19

[Andreas Gal :gal]

Patch is pretty straight forward and is easy to back out. Its a security check and its covered by a test.

Comment 20

[David Mandelin [:dmandelin]]

Comment on attachment 492539 [details] [diff] [review]
patch

OK, we did really want this, let's do it.

Comment 21

[imradyurrad]

Is somebody gonna check this in?

Comment 22

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/a4b72ddb9e6e

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a4b72ddb9e6e