Bug 18749 – SQUIRRELFISH: const support is broken

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Strangely enough, run-sunspider --runs 40 says this patch is an approximately 0.5% progression.

[Cameron Zwarich (cpst)]

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.

[Oliver Hunt]

Comment on attachment 20885 [details]
Revised proposed patch

Okay this looks basically fine, but id really like geoff to look at it.

[Cameron Zwarich (cpst)]

Created attachment 20905 [details]
Revised proposed patch

Here is a new version that patches cleanly against ToT.

[Geoffrey Garen]

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.

[Cameron Zwarich (cpst)]

(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.

[Geoffrey Garen]

> > 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.

[Cameron Zwarich (cpst)]

On cue, this patch is now a 0.3% regression. I don't think it's real because of the progression before.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Comment on attachment 20964 [details]
Revised proposed patch

The layout tests are now fixed, and this patch indeed fixes the const tests.

[Oliver Hunt]

Committed r32866