Summary: | SQUIRRELFISH: const support is broken | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cameron Zwarich (cpst) <zwarich> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Cameron Zwarich (cpst) <zwarich> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ggaren, mjs, oliver | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 18631 | ||||||||||||||
Attachments: |
|
Description
Cameron Zwarich (cpst)
2008-04-25 18:51:23 PDT
To pass the layout tests, we only need to do the case of locals and this: var object = { inWith1: "RIGHT", inWith2: ""} with (object) { const inWith1 = "WRONG"; const inWith2 = "RIGHT"; inWith2 = "WRONG"; } shouldBe("object.inWith1", "'RIGHT'"); shouldBe("inWith2", "'RIGHT'"); We also don't need to worry about something like this, because it's broken in trunk: const x = 1; var x = 2; const y = 1; const y = 2; I'm assigning this to myself. Created attachment 20881 [details]
Patch in progress
Here is a patch that implements all of the necessary codegen for const. It passes almost all of the const tests, the only exception being things of the form:
const x = 1;
function f() { x = 2; }
This is because the attributes are not respected at runtime in JSVariableObject. I'm not quite sure of the best way to store them.
Comment on attachment 20881 [details]
Patch in progress
Removing review flag. I am going to make it store the ReadOnly bit in the index in the SymbolTable.
Created attachment 20884 [details]
Proposed patch
Here's a patch that fixes the 2 const tests and doesn't regress on any other tests. It works, but I think it would be served with some better abstraction of the indices in the SymbolTable from the actual entries.
I am going to do some performance testing now. If stealing a bit is too expensive then we may need to have two separate hash tables for non-const and const entries.
Strangely enough, run-sunspider --runs 40 says this patch is an approximately 0.5% progression. Created attachment 20885 [details]
Revised proposed patch
I needed to make a small change in order to get the patch to compile in release mode.
Comment on attachment 20885 [details]
Revised proposed patch
Okay this looks basically fine, but id really like geoff to look at it.
Created attachment 20905 [details]
Revised proposed patch
Here is a new version that patches cleanly against ToT.
This patch looks good to me, with one caveat: I'd really like to see SymbolTable become a true class that has a HashMap data member, and does the bit masking for you automatically. Right now, SymbolTable is kind of error-prone, since all clients of get, set, and add need to properly apply the secret bit masking magic. So, I'd suggest writing a follow-on patch to turn SymbolTable into a true class. (In reply to comment #9) > This patch looks good to me, with one caveat: I'd really like to see > SymbolTable become a true class that has a HashMap data member, and does the > bit masking for you automatically. Right now, SymbolTable is kind of > error-prone, since all clients of get, set, and add need to properly apply the > secret bit masking magic. Yeah, I asked in my original request for review for the best way to remove that problem. I thought about it a bit last night, and came to the same solution you proposed, so that's probably best. > So, I'd suggest writing a follow-on patch to turn SymbolTable into a true > class. I might as well do it before I ask anyone to land it for me. > > So, I'd suggest writing a follow-on patch to turn SymbolTable into a true
> > class.
>
> I might as well do it before I ask anyone to land it for me.
OK, I'll clear the review bit and hold off on landing for now.
On cue, this patch is now a 0.3% regression. I don't think it's real because of the progression before. Created attachment 20964 [details] Revised proposed patch Here is a revised version of my patch, using the attributes support added in r32859. I didn't think that approach would have good enough performance, but thankfully it seems to be fine. This doesn't change any JavaScriptCore test results, and I couldn't actual run the layout tests, because r32859 broke JS in an actual browser window, but the const tests work fine when I run them on the command line. After the tests are fixed, I'll run them and ask for review. Comment on attachment 20964 [details]
Revised proposed patch
The layout tests are now fixed, and this patch indeed fixes the const tests.
Committed r32866 |