489098 - enabling property cache for eval scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently the property cache is disabled for eval scripts. The reason for the ban was that the eval code was eagerly deleted right after execution which would make pc cache entries no longer valid. Another problem was related to the indirect eval and the with block it added to the scope chain.

But such reasoning is no longer valid. Now days eval scripts and destroyed during the GC and the name lookup code never caches any with blocks or other non-white listed non-globals on the scope chain, see bug 487039. Moreover, the property cache could be rather effective due to caching of the eval scripts. So the ban should be reconsidered.

Comment 1

[Brendan Eich [:brendan]]

Would this help our SS score a bit? Easy to test.

/be

Comment 2

[Brendan Eich [:brendan]]

TraceRecorder::record_JSOP_POPV() has some code that depends on eval code not being traced.

/be

Comment 3

[Andreas Gal :gal]

This sounds pretty important. Can we get this done for FF4?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

FWIW, JM will emit PICs for eval scripts regardless of what the property cache decides.

(In reply to comment #2)

We trace some loops inside eval though, right? I'm curious (because of plan to remove rval) what the conflict is.

Comment 5

[Andreas Gal :gal]

We can't trace if the property cache is off.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

If the trace doesn't hit the property cache though, it'll compile.

Comment 7

[Andreas Gal :gal]

yeah well you can't do much that doesn't access some sort of global property

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #4)
> FWIW, JM will emit PICs for eval scripts regardless of what the property cache
> decides.

This bug is fixable. Ground work laid a long time ago:

changeset:   18282:3bb542e73570
parent:      18279:163eb5c5f0de
user:        Brendan Eich <[e-mail]>
date:        Wed Aug 20 22:18:42 2008 -0700
summary:     Defer eval'ed script destruction to next GC; expose js_obj_eval for tracing.

We just never followed up on it (see the FIXME citing this bug 489098 in js::PropertyCache::fill).


> (In reply to comment #2)
> 
> We trace some loops inside eval though, right? I'm curious (because of plan to
> remove rval) what the conflict is.

There's no inherent conflict. An eval call has to capture a completion value. The comment in TR::record_JSOP_POPV should be clarified to talk about tracing from an outer frame through an inlined function or two then into an eval call, since inlining means there's no frame whose rval we can box_value_into -- but we wouldn't do that anyway.

Whatever we do for rval ought to work for both JITs.

/be

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

JM needs work here too. Filed related bug 581291 for PICs across eval().

Comment 10

[Paul Biggar]

Attachment: Remove pessimistic assumption

This enables the property cache for eval. After this change, the massive changes I reported in bug 580529 go away.

That said, I have no idea what I'm doing here. The reftests and trace-tests pass, so I would guess I haven't broken anything?

With regard the comment in TraceRecorder::record_JSOP_POPV, I don't know what to change it to (assuming that comment 8 is saying we _only_ need to change the comment). Suggestions?


This leads to no (discernable) speedup in SS or v8, despite 4 subbenchmarks using eval.

Comment 11

[Paul Biggar]

dvander: I believe I'm right in thinking that the JM work required here is already done?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #11)

Yeah. JM only uses the property cache as a fallback from PICs.

Comment 13

[Andreas Gal :gal]

Comment on attachment 464490 [details] [diff] [review]
Remove pessimistic assumption

Aehm is this safe now? I have to read up on 489098.

Comment 14

[Paul Biggar]

(In reply to comment #13)
> Aehm is this safe now? I have to read up on 489098.

This is 489098.

I'm pretty sure I ran tests, etc, but I'll rebase and run them again.

Comment 15

[Brendan Eich [:brendan]]

This has been safe for a while! We GC eval-created scripts (cache 'em that way too). I can't think of other probs.

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 464490 [details] [diff] [review]
Remove pessimistic assumption

We need the js_IsPropertyCacheDisabled test still, tho.

/be

Comment 17

[Paul Biggar]

Attachment: No longer stop tracing evals + tests

Fix it properly. I misunderstood the problem before.

This no longer turns off the tracer when an eval is seen. However, my use case (running sunspider via eval) has brought up a couple of asserts from bug 589318. I've attached a test for 589318.

Luke: Is there a way to do both this and the cleanup related to bug 589318? I know there's an anti-eval vibe, but I think tracing into eval is pretty important for real things on the web.

Comment 18

[Luke Wagner [:luke]]

(In reply to comment #17)
Yeah, totally.  Bug 589318 just wants us not asking about a frame's arguments if that frame is an eval, which is usually easy to fix (e.g., see numEntryFrameArgs).  I meant to fix all these, but it seems my fix was undertested and now I see why.  I'll apply your patch and see if I can't fix em all.

Comment 19

[Luke Wagner [:luke]]

Attachment: fix

Attachment 468304 [details] [diff] passes trace tests with this patch applied.

Comment 20

[Paul Biggar]

Attachment: Combined patch

Combined patch, rebased and retested. Also checked sunspider when using eval - no slowdown.

Comment 21

[Paul Biggar]

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

Comment 22

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/dc01b8dbb58a