Bug 18518 - Implement eval in SquirrelFish
Summary: Implement eval in SquirrelFish
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-15 13:40 PDT by Cameron Zwarich (cpst)
Modified: 2008-04-17 14:08 PDT (History)
3 users (show)

See Also:


Attachments
Patch in progress (7.88 KB, patch)
2008-04-15 17:28 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised patch in progress (15.68 KB, patch)
2008-04-15 19:33 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised patch in progress (15.60 KB, patch)
2008-04-16 09:05 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (15.02 KB, patch)
2008-04-16 09:59 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Reduction of crash (160 bytes, text/plain)
2008-04-16 13:41 PDT, Cameron Zwarich (cpst)
no flags Details
Revised patch in progress (16.54 KB, patch)
2008-04-16 16:49 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised patch in progress (16.69 KB, patch)
2008-04-16 17:22 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised patch in progress (18.88 KB, patch)
2008-04-16 17:49 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised patch in progress (18.90 KB, patch)
2008-04-16 19:12 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
patch (18.35 KB, patch)
2008-04-16 22:45 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
patch (18.38 KB, patch)
2008-04-16 23:58 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
patch (18.32 KB, patch)
2008-04-17 00:08 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
patch (18.97 KB, patch)
2008-04-17 01:01 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
patch (19.75 KB, patch)
2008-04-17 09:53 PDT, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-04-15 13:40:55 PDT
This is the main blocker for the remaining SunSpider tests.
Comment 1 Cameron Zwarich (cpst) 2008-04-15 17:28:02 PDT
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.
Comment 2 Cameron Zwarich (cpst) 2008-04-15 19:33:11 PDT
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.
Comment 3 Cameron Zwarich (cpst) 2008-04-16 09:05:22 PDT
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.
Comment 4 Cameron Zwarich (cpst) 2008-04-16 09:59:26 PDT
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().
Comment 5 Cameron Zwarich (cpst) 2008-04-16 13:41:48 PDT
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.
Comment 6 Geoffrey Garen 2008-04-16 14:42:24 PDT
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.
Comment 7 Geoffrey Garen 2008-04-16 14:44:39 PDT
...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".
Comment 8 Cameron Zwarich (cpst) 2008-04-16 16:49:09 PDT
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.
Comment 9 Cameron Zwarich (cpst) 2008-04-16 17:22:51 PDT
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.
Comment 10 Cameron Zwarich (cpst) 2008-04-16 17:49:53 PDT
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.
Comment 11 Cameron Zwarich (cpst) 2008-04-16 19:12:11 PDT
Created attachment 20610 [details]
Revised patch in progress

Here's the latest. Since Maciej landed his GC patch, it now runs all SunSpider tests.
Comment 12 Geoffrey Garen 2008-04-16 21:23:12 PDT
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
Comment 13 Geoffrey Garen 2008-04-16 22:45:18 PDT
Created attachment 20613 [details]
patch

I think this patch is almost ready to land. I measure no substantial sunspider regression.
Comment 14 Geoffrey Garen 2008-04-16 22:46:39 PDT
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.
Comment 15 Geoffrey Garen 2008-04-16 23:55:54 PDT
(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.)
Comment 16 Geoffrey Garen 2008-04-16 23:58:33 PDT
Created attachment 20614 [details]
patch

Added support for "this" inside eval. Removed the performance-sensitive line of code I mentioned above.
Comment 17 Geoffrey Garen 2008-04-17 00:08:30 PDT
Created attachment 20615 [details]
patch

Added support for throwing an exception after a syntax error (doesn't seem to be a regression anymore).
Comment 18 Geoffrey Garen 2008-04-17 01:01:04 PDT
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.
Comment 19 Cameron Zwarich (cpst) 2008-04-17 07:16:05 PDT
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.
Comment 20 Cameron Zwarich (cpst) 2008-04-17 07:33:32 PDT
Doh, the mornings can be slow. I should really read all of Geoff's posts before posting myself.
Comment 21 Geoffrey Garen 2008-04-17 09:53:44 PDT
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.
Comment 22 Oliver Hunt 2008-04-17 14:05:51 PDT
Comment on attachment 20630 [details]
patch

The indirect goto hack makes me cry :(

I wouldn't mind an ASSERT(!exec->dynamicGlobalObject()->debugger()) 
 
in globalFuncEval
Comment 23 Geoffrey Garen 2008-04-17 14:08:22 PDT
Committed revision 32013.