548671 - Shared-permanent properties aren't necessary for implementing [].length or (function(){}).length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Shared-permanent properties, while in some cases usefully avoiding some memory cost, make determination of the own properties of an object (and of whether a property is an own property) complicated and unexpected given what the spec would lead one to assume.  If we were ever to make this simplification to our code, it would require removing instances of the pattern from SpiderMonkey.

Here's a patch to remove use of shared-permanent for representing array and function lengths.  It doesn't seem necessary to use shared-permanent in this case, and using it increases line counts a bit, even given this patch's changes to JSOP_LENGTH.  Regardless whether shared-permanent properties ever go away, this patch seems independently useful.  SunSpider suggests the performance change of this varies from a relative wash to very slightly faster.

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Regardless whether shared-permanent properties ever go away,
> this patch seems independently useful.  SunSpider suggests the performance
> change of this varies from a relative wash to very slightly faster.

SunSpider is very noisy, but the only slow arrays are are one in 3d-cube.js (due to a named proeprty, "Edge"), and four in 3d-raytrace.js ('reflection' and three 'colour' properties). The patch now binds 'length' for those, but it is in the noise (it's not a perf win :-/).

This looks like a good patch to me, at a quick read.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 429009 [details] [diff] [review]
Patch

In array_length_getter:
>+    JS_ASSERT(obj->isArray());

You can trip this assertion by doing:

  var a = [];
  a.p = 1;
  var x = Object.create(a);
  x.length;

Even non-SHARED getters have to cope with thisobj of any class.

I think non-SHARED setters are safe, but I'm not sure.

In fun_resolve:
>+    atom = cx->runtime->atomState.lengthAtom;
>+    if (id == ATOM_KEY(atom)) {
>+        JS_ASSERT(!js_IsInternalFunctionObject(obj));
>+        if (!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),

You can use `ATOM_TO_JSID(atom)` on that last line.

In jsops.cpp, CASE(JSOP_LENGTH):
>+        if (OBJ_IS_ARRAY(cx, obj)) {

`obj->isArray()`?

>+        } else if (obj->isFunction()) {
>+            length = GET_FUNCTION_PRIVATE(cx, obj)->nargs;

Is this special case special enough? Your call. (There are always moderately compelling reasons not to push so hard, even when it's just two straightforward lines. A correct fast path can conceal a broken slow path from tests and fuzzers, for example.)

r=me with appropriate changes.

Comment 3

[Igor Bukanov]

Non-shared getters and setters may cause unexpected memory leaks, see bug 488458. We should fix that first before considering this patch.

Comment 4

[Igor Bukanov]

(In reply to comment #0)
> Shared-permanent properties, while in some cases usefully avoiding some memory
> cost, make determination of the own properties of an object (and of whether a
> property is an own property) complicated and unexpected given what the spec
> would lead one to assume.

Could you write why shared properties are problematic?

Comment 5

[Jeff Walden [:Waldo]]

They're problematic because you can't simply determine what properties are on an object by going from scope->lastProp and following parent links.  You have to also look at all the properties on the prototype object, filter out the ones which are shared-permanent, determine whether they exist on the original object or not, and add them to the list if they don't.  This uniqueness requirement means the properties can't always simply be splatted into an array to be returned, at least not without O(n**2) behavior once you have to start checking for shared-permanent.  You also start needing multiple object locks once you do this.

jorendorff also pointed out in bug 430133 comment 65 that shared-permanent has bad interactions with user-defined getters:

  function C() {}
  Object.defineProperty(C.prototype, "x", {get: function () { return 0; }});
  assertEq(new C().hasOwnProperty("x"), false);  // fails
  assertEq(Object.getOwnPropertyDescriptor(new C), undefined);  // succeeds

Removing JSPROP_SHARED from the attributes for defined-as-accessor properties isn't feasible right now from what I understand, which means we're stuck with the above behavior for the moment.

In any case, I've gotten sidetracked a little by the problem of dense array property lookups, so this isn't quite ready yet regardless.  (I could have sworn I ran tests on this before posting, not sure what happened exactly here.)  The patch is currently tripping over this testcase, reduced from ecma_5/Object/15.2.3.6-miscellaneous.js:

  var a = Object.defineProperty([],  0, { value: 17 });
  Object.getOwnPropertyDescriptor(a, "length");
  assertEq(a.length, 1);

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> They're problematic because you can't simply determine what properties are on
> an object by going from scope->lastProp and following parent links.  You have
> to also look at all the properties on the prototype object, filter out the ones
> which are shared-permanent, determine whether they exist on the original object
> or not, and add them to the list if they don't.

This is not the problem with shared properties. The issue is that ES5 requires that .length should be own property of the array and other objects. A shared property on the prototype does not reflect that. But this can be fixed with adding the property to the array itself. It should not matter if the property is shared or not. 

> jorendorff also pointed out in bug 430133 comment 65 that shared-permanent has
> bad interactions with user-defined getters:
> 
>   function C() {}
>   Object.defineProperty(C.prototype, "x", {get: function () { return 0; }});
>   assertEq(new C().hasOwnProperty("x"), false);  // fails
>   assertEq(Object.getOwnPropertyDescriptor(new C), undefined);  // succeeds

Hm, I do not see how is this different from having a read-only property on the prototype and then assigning it on the object? It silently fails since the very beginning of ES. Also, how would non-shared permanent property help in the above example?

Comment 7

[Jason Orendorff [:jorendorff]]

To clarify my position:

The effect of JSPROP_SHARED | JSPROP_PERMANENT isn't a simple composition of the effects that the two flags have separately. The SHARED PERMANENT hack has been posing as a clever transparent optimization for years, but that's emphatically not what it is. It changes API-visible semantics. It's an undocumented oddity in the JSAPI. The comment in js_HasOwnProperty that says "It's not really a hack, of course" is wrong. Laughable, in fact, once you get your head screwed on right!  ;-)

Moreover, I think this hack has been causing script-visible correctness bugs in corner cases for as long as it has existed. I discovered a few just now.

  var a = [1, 2, 3];
  a.__proto__ = null;
  assertEq("length" in a, true);  // fails
  assertEq(Object.hasOwnProperty.call(a, "length"), true); // fails
  assertEq(a.length, 3);  // passes!

Even more mysteriously:

  var a = [], b = [];
  b.__proto__ = a;
  assertEq(b.hasOwnProperty("length"), true);  // fails
  b.length = 42; // no idea what this is doing, nothing apparently
  assertEq(b.length, 42); // fails

Now that scripts can declare SHARED PERMANENT properties (using Object.defineProperty), you don't have to go that far afield to find correctness bugs. We are in clear violation of the standard here:

>   function C() {}
>   Object.defineProperty(C.prototype, "x", {get: function () { return 0; }});

ES5 says that this Object.defineProperty call creates an own property on C.prototype. It must not later appear to be an own property of new C instances. 

>   assertEq(new C().hasOwnProperty("x"), false);  // fails

hasOwnProperty must return false here. We return true.

>   assertEq(Object.getOwnPropertyDescriptor(new C), undefined);  // succeeds

This second assertion passes due to a typo (my typo in bug 430133 comment 65, which Waldo quoted). Correcting it:

  assertEq(Object.getOwnPropertyDescriptor(new C, "x"), undefined);  // fails

Same bug again. Per ES5, we should return undefined. Instead we return a description of C.prototype.x.

Deleting the SHARED PERMANENT hack would fix all these bugs.

Comment 8

[Jason Orendorff [:jorendorff]]

Waldo, I think your bug is that you pass JSSLOT_ARRAY_LENGTH to addProperty. As a result, each time the length getter or setter is called, the jsval it leaves in *vp gets stored in that slot -- overwriting the true length, which is not a jsval. This is a pretty bad bug. I think the only answer is to make the length property SHARED as Igor proposes.

But note that SHARED affects property assignment (see <https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_SetProperty>). To get the right behavior, you would have to keep the if (!OBJ_IS_ARRAY(...)) check in array_length_setter.

Comment 9

[Jeff Walden [:Waldo]]

Yeah, we were storing the length as a tagged jsval in that slot -- hence (1<<1)|1 rather than 1 for the length.  (Er, looking back, I see I didn't actually mention the bug here, just on IRC; here's the failing testcase, derived from an existing test in ecma_5/Object/:)

  var o = [];
  Object.defineProperty(o, 0, { value: 17 });
  Object.getOwnPropertyDescriptor(o, "length"); // does slot store noted above
  assertEq(o.length, 1); // got 3, expected 1

In any case, this is somewhat backburnered now while I work on ES5 feature work.  I had thought this to be simple enough to do in passing, and maybe the necessary tweaks are moderately straightforward, but this is less pressing than that work.

Comment 10

[Jason Orendorff [:jorendorff]]

To repeat what I said on IRC today: I think it's worth making these .length properties true own properties (lazily resolved), even if they remain both SHARED and PERMANENT. It would fix some of the correctness bugs, and it would ensure that these properties would continue to work properly when we do remove the SHARED PERMANENT hack (which will fix the rest).

Comment 11

[Brendan Eich [:brendan]]

Didn't Igor say that in comment 6?

/be

Comment 12

[Jason Orendorff [:jorendorff]]

If so, he didn't convince jwalden, who by yesterday seemed to have decided it wasn't worth finishing if he couldn't make the properties non-SHARED.

I couldn't tell what comment 6 was getting at, especially the second part.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> I couldn't tell what comment 6 was getting at, especially the second part.

>   function C() {}
>   Object.defineProperty(C.prototype, "x", {get: function () { return 0; }});
>   assertEq(new C().hasOwnProperty("x"), false);  // fails
>   assertEq(Object.getOwnPropertyDescriptor(new C), undefined);  // succeeds

Suppose C.prototype contains read-only property x wit value 1. Then we have in ES3:
  
var c = new C(); 
c.x = 2; // silently fails
assertEq(c.x, 2);  // fails
assertEq(c.x, 1);  // succeeds
assertEq(c.hasOwnProperty("x"), false);  // fails
assertEq(Object.getOwnPropertyDescriptor(c), undefined);  // succeeds

I do not see how the example from the comment 5 is particularly different in this regard.

Comment 14

[Igor Bukanov]

I think the issue is the mismatch between the SM practice of using a native getter on the prototype between ES5 requirement that functions and arrays owns the length property. That practice avoids bloating the object with extra properties for the cost of ES5 incompatibility. It does not matter if the getter is shared or non-shared one. As long as it is defined only on the prototype, we are going to have this mismatch.

If the issue is only with the length property, than we can simply fix this via adding the resolve hook for arrays and functions and hope that the length property specialization in the code would trigger that hook only in pathological cases.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 429009 [details] [diff] [review]
Patch

Yes. I think we all agree, then.

Minusing the known-bad patch to avoid confusion.

Comment 16

[Brendan Eich [:brendan]]

Applies to String.prototype.length too.

/be

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to comment #13)
> Suppose C.prototype contains read-only property x wit value 1. Then we have in
> ES3:
> 
> var c = new C(); 
> c.x = 2; // silently fails
> assertEq(c.x, 2);  // fails
> assertEq(c.x, 1);  // succeeds
> assertEq(c.hasOwnProperty("x"), false);  // fails
> assertEq(Object.getOwnPropertyDescriptor(c), undefined);  // succeeds
> 
> I do not see how the example from the comment 5 is particularly different in
> this regard.

Just that the above behavior complies with ES3 and ES5, but our behavior in the example from comment 5 doesn't. Comment 7 takes it apart and explains. Basically it's exactly what you said in comment 14.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Own but still SHARED

Somehow I managed to completely misread the above comments saying to make the property per-object but still shared.  With that change, things all work correctly, and all tests pass.  How about it?

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 449076 [details] [diff] [review]
Own but still SHARED

I would've liked it better without the JSObject::makeDenseArraySlow change, but ok.

In JSObject::makeDenseArraySlow:
>+    /* Begin with the length property to share more of the property tree. */
>+    if (!scope->addProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),
>+                            array_length_getter, array_length_setter,
>+                            JSSLOT_ARRAY_LENGTH, JSPROP_PERMANENT | JSPROP_SHARED, 0, 0)) {
>+        goto out_bad;
>+    }

Someday we could cache a JSScopeProperty for this and use scope->extend() to save some time here.

>+    atom = cx->runtime->atomState.lengthAtom;
>+    if (id == ATOM_KEY(atom)) {

I think that should be ATOM_TO_JSID(atom) on the right-hand side. (Symptomless bug.)

>+        JS_ASSERT(!IsInternalFunctionObject(obj));
>+        if (!js_DefineNativeProperty(cx, obj, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),

That too.

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> I would've liked it better without the JSObject::makeDenseArraySlow change,
> but ok.

JSSLOT_ARRAY_LENGTH is private now, which is why I had to make the change.  The method really wants to only be on a DenseArray class, but this as an incremental step makes sense along with njn's incremental adjustments in this area.

Comment 21

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/b3e27c1ee35e

Comment 22

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/mozilla-central/rev/b3e27c1ee35e