Bug 18518 – Implement eval in SquirrelFish

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Cameron Zwarich (cpst)]

This is the main blocker for the remaining SunSpider tests.

[Cameron Zwarich (cpst)]

Created attachment 20572 [details]
Patch in progress

Here is a patch that implements indirect eval. I will try to implement direct eval (which is more useful for SunSpider) shortly and upload a new patch soon.

[Cameron Zwarich (cpst)]

Created attachment 20574 [details]
Revised patch in progress

Here is a patch in progress that implements direct eval, except it doesn't pass in the correct variable object. It also has no error handling at all. I'll hopefully upload a new patch later that works as planned.

[Cameron Zwarich (cpst)]

Created attachment 20586 [details]
Revised patch in progress

Here's a patch now have support for direct eval. Since there is not necessarily a correct ScopeChain, I just had to change the eval Machine::execute method to take a ScopeChainNode* and it worked fine. All of the SunSpider tests now run, but I can't benchmark due to an existing SunSpider crash in copyRegisters(). Also, exception handling is still missing.

[Cameron Zwarich (cpst)]

Created attachment 20588 [details]
Revised proposed patch

Here's a revised version of the patch that is a bit cleaner and implements some exception handling. It also fixes a stupid bug in the last patch, and now 2 of the 3 SunSpider tests using eval crash. The date-format-tofte test calls getCallData() on a null pointer in one of the eval'd calls. This one should be the easiest to debug. The string-tagcloud test dies in getPropertySlot().

[Cameron Zwarich (cpst)]

Created attachment 20602 [details]
Reduction of crash

This is a reduction of the date-format-tofte crash. The crash occurs in the lookup of f(). It no longer occurs if f() is made a global rather than a local, so my guess is that the variable is just being clobbered by something I am doing wrong.

[Geoffrey Garen]

Comment on attachment 20588 [details]
Revised proposed patch

I haven't figured out the exact cause of the crash, but here are some preliminary comments on the patch:

+{
+    m_locals.resize(1);
+    addVar(m_propertyNames->thisIdentifier);
+}

There's no need to explicitly resize m_locals unless you want to place locals at arbitrary indices in m_locals, like addParameter does. Since you're just using addVar, which appends to m_locals, you can leave the resize call out.

+JSValue* Machine::execute(EvalNode* evalNode, ExecState* exec, RegisterFileStack* registerFileStack, ScopeChainNode* scopeChain, JSValue** exception)
+{
+    RegisterFile* registerFile = registerFileStack->pushRegisterFile();
+

pushRegisterFile is a heavy-weight operation meant to enable declaration of new global symbols during re-entrant evaluation of global code. Eval doesn't declare any symbols, and it isn't guaranteed to execute in global scope, either, so it shouldn't use pushRegisterFile. It should just move "r", like a function call does, to create a new register frame.

Be sure to shrink the register file to its old size before returning. Otherwise, repeated calls to eval will cause the register file to grow without bound. (Maciej just fixed a similar bug for function calls.)

There sure is a lot of duplicated code between op_call and op_eval. I hope we can figure out how to eliminate that duplication, or at least move the duplicated code into a shared ALWAYS_INLINE or template function.

[Geoffrey Garen]

...also, as we discussed on IRC, it's not appropriate for eval to use the global object's symbol table. Eval should just create its own symbol table, with one entry: "this".

[Cameron Zwarich (cpst)]

Created attachment 20607 [details]
Revised patch in progress

Here is a patch incorporating Geoff's comments. It fixes the crash in date-format-tofte in a debug build. In a release build, it eventually crashes, but it is seemingly generating the correct output until it crashes. The date-format-xparb test runs fine in both Debug and Release, and the string-tagcloud test crashes.

I had to disable indirect eval, because I am not sure how to get the all of the register information so I can do things without RegisterFileStack. The value of 'this' is also not set correctly.

[Cameron Zwarich (cpst)]

Created attachment 20608 [details]
Revised patch in progress

Here is a patch incorporating the things we talked about on IRC. It fixes the crash in date-format-tofte, so now both the date tests run fine and give correct results. The string-tagcloud test still crashes. I'll add global eval again with our new scheme, and have another patch shortly.

[Cameron Zwarich (cpst)]

Created attachment 20609 [details]
Revised patch in progress

Here is the latest patch. I reimplemented indirect eval in a manner similar to JS -> native -> JS function calls. The only things missing featurewise are setting the correct value of 'this', throwing exceptions on syntax errors (which I will add in the next patch), and propagating exceptions from an eval context to the current context (I think I know how to do this properly, but Oliver volunteered to do it after I am finished with this patch).

Maciej and I are debugging the string-tagcloud crash. At this point, it looks like it is probably a ScopeChain GC problem.

[Cameron Zwarich (cpst)]

Created attachment 20610 [details]
Revised patch in progress

Here's the latest. Since Maciej landed his GC patch, it now runs all SunSpider tests.

[Geoffrey Garen]

I tried the following gcc options, with no measurable gain:

--param max-goto-duplication-insns=80 --param max-reload-search-insns=1000 --param max-cselib-memory-locations=5000 --param max-sched-region-blocks=100 --param max-sched-region-insns=1000 --param max-last-value-rtl=100000 --param max-cse-path-length=100 --param max-delay-slot-insn-search=500

[Geoffrey Garen]

Created attachment 20613 [details]
patch

I think this patch is almost ready to land. I measure no substantial sunspider regression.

[Geoffrey Garen]

By the way, I'm pretty sure this change

-            r[argv].u.jsValue = r2 == missingSymbolMarker() ? jsNull() : (r[r2].u.jsValue)->toObject(exec); // "this" value
+            r[argv].u.jsValue = r2 == missingSymbolMarker() ? exec->lexicalGlobalObject() : (r[r2].u.jsValue)->toObject(exec); // "this" value

has a performance cost. We might want to figure out a way to land it separately.

[Geoffrey Garen]

(In reply to comment #14)

I measured, and the cost to that change seems to be 11ms, or .5%. So yeah, we should land it separately. (Maciej and I came up with an idea for how to do "this" object transmutation lazily, to avoid the slowdown.)

[Geoffrey Garen]

Created attachment 20614 [details]
patch

Added support for "this" inside eval. Removed the performance-sensitive line of code I mentioned above.

[Geoffrey Garen]

Created attachment 20615 [details]
patch

Added support for throwing an exception after a syntax error (doesn't seem to be a regression anymore).

[Geoffrey Garen]

Created attachment 20616 [details]
patch

Cleaned up some naming, fixed compilation in the switch case. Not perfect, but I think this might be ready to land.

[Cameron Zwarich (cpst)]

I can confirm that there is no performance difference on my machine with networking on. I'll try to use the correct value for 'this' and throw a syntax error (the hardest part is formatting it). I have to proctor and mark an exam later today, so I'll post what I get and hand it off to somebody else.

[Cameron Zwarich (cpst)]

Doh, the mornings can be slow. I should really read all of Geoff's posts before posting myself.

[Geoffrey Garen]

Created attachment 20630 [details]
patch

Since the patch worked for Cameron, I think I'll put it up for review.

New changes in this version: Fixed exception propagation from "this.eval". Changed eval codegen to use resolve_base_and_func instead of resolve_base_and_property, so "this" is right if eval is declared as a local in a function. Renamed emitEval to emitCallEval, to match the earlier opcode rename I did.

[Oliver Hunt]

Comment on attachment 20630 [details]
patch

The indirect goto hack makes me cry :(

I wouldn't mind an ASSERT(!exec->dynamicGlobalObject()->debugger()) 
 
in globalFuncEval

[Geoffrey Garen]

Committed revision 32013.