Bug 18749 - SQUIRRELFISH: const support is broken
Summary: SQUIRRELFISH: const support is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks: 18631
  Show dependency treegraph
 
Reported: 2008-04-25 18:51 PDT by Cameron Zwarich (cpst)
Modified: 2008-05-05 01:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch in progress (11.49 KB, patch)
2008-04-28 18:11 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (22.83 KB, patch)
2008-04-29 00:39 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (22.84 KB, patch)
2008-04-29 01:22 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (22.80 KB, patch)
2008-04-30 13:25 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (9.91 KB, patch)
2008-05-04 21:16 PDT, Cameron Zwarich (cpst)
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-25 18:51:23 PDT
We currently emit code for const, but we don't check whether something is const before assigning to it. While there are ways around this in trunk JavaScriptCore as well, we should at least try to match the same level of support. This affects the layout tests fast/js/const and fast/js/kde/const.
Comment 1 Cameron Zwarich (cpst) 2008-04-27 21:00:39 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.
Comment 2 Cameron Zwarich (cpst) 2008-04-28 18:11:49 PDT
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 3 Cameron Zwarich (cpst) 2008-04-28 18:40:55 PDT
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.
Comment 4 Cameron Zwarich (cpst) 2008-04-29 00:39:12 PDT
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.
Comment 5 Cameron Zwarich (cpst) 2008-04-29 00:53:21 PDT
Strangely enough, run-sunspider --runs 40 says this patch is an approximately 0.5% progression.
Comment 6 Cameron Zwarich (cpst) 2008-04-29 01:22:47 PDT
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 7 Oliver Hunt 2008-04-29 22:40:43 PDT
Comment on attachment 20885 [details]
Revised proposed patch

Okay this looks basically fine, but id really like geoff to look at it.
Comment 8 Cameron Zwarich (cpst) 2008-04-30 13:25:52 PDT
Created attachment 20905 [details]
Revised proposed patch

Here is a new version that patches cleanly against ToT.
Comment 9 Geoffrey Garen 2008-04-30 13:37:46 PDT
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.
Comment 10 Cameron Zwarich (cpst) 2008-04-30 13:45:42 PDT
(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.
Comment 11 Geoffrey Garen 2008-04-30 13:56:51 PDT
> > 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.
Comment 12 Cameron Zwarich (cpst) 2008-04-30 14:01:29 PDT
On cue, this patch is now a 0.3% regression. I don't think it's real because of the progression before.
Comment 13 Cameron Zwarich (cpst) 2008-05-04 21:16:10 PDT
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 14 Cameron Zwarich (cpst) 2008-05-04 21:54:38 PDT
Comment on attachment 20964 [details]
Revised proposed patch

The layout tests are now fixed, and this patch indeed fixes the const tests.
Comment 15 Oliver Hunt 2008-05-05 01:05:23 PDT
Committed r32866