Closed Bug 489098 Opened 13 years ago Closed 12 years ago

enabling property cache for eval scripts

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: paul.biggar)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 3 obsolete files)

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.
Would this help our SS score a bit? Easy to test.

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

/be
This sounds pretty important. Can we get this done for FF4?
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.
We can't trace if the property cache is off.
If the trace doesn't hit the property cache though, it'll compile.
yeah well you can't do much that doesn't access some sort of global property
(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 <brendan@mozilla.org>
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
Blocks: 580529
JM needs work here too. Filed related bug 581291 for PICs across eval().
Assignee: general → pbiggar
Attached patch Remove pessimistic assumption (obsolete) — Splinter Review
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.
Attachment #464490 - Flags: feedback?(brendan)
dvander: I believe I'm right in thinking that the JM work required here is already done?
(In reply to comment #11)

Yeah. JM only uses the property cache as a fallback from PICs.
Attachment #464490 - Flags: feedback?(brendan) → feedback?(gal)
Comment on attachment 464490 [details] [diff] [review]
Remove pessimistic assumption

Aehm is this safe now? I have to read up on 489098.
(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.
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 on attachment 464490 [details] [diff] [review]
Remove pessimistic assumption

We need the js_IsPropertyCacheDisabled test still, tho.

/be
Attachment #464490 - Flags: feedback?(gal) → feedback-
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.
Attachment #464490 - Attachment is obsolete: true
Attachment #468304 - Flags: feedback?(lw)
(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.
Attached patch fix (obsolete) — Splinter Review
Attachment 468304 [details] [diff] passes trace tests with this patch applied.
Attachment #468394 - Flags: review?(dvander)
Attached patch Combined patchSplinter Review
Combined patch, rebased and retested. Also checked sunspider when using eval - no slowdown.
Attachment #468304 - Attachment is obsolete: true
Attachment #468394 - Attachment is obsolete: true
Attachment #468650 - Flags: review?(dvander)
Attachment #468304 - Flags: feedback?(lw)
Attachment #468394 - Flags: review?(dvander)
Attachment #468650 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/dc01b8dbb58a
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/dc01b8dbb58a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.