Closed Bug 558451 Opened 14 years ago Closed 14 years ago

Merge JSScope into JSScopeProperty, JSObject

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b3

People

(Reporter: jorendorff, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(7 files, 43 obsolete files)

4.92 KB, text/plain
Details
399.21 KB, application/x-gzip
Details
744.77 KB, patch
Details | Diff | Splinter Review
33.47 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.19 MB, text/plain
Details
1.01 MB, text/plain
Details
13.62 KB, text/plain
Details
See bug 555128 comment 4 for a partly worked-out description of what this would look like.
Another idea for overriding shape induced by tree of properties lexicographically constructed on alphabet of "property data (excluding shape)":

Add new nodes with same property data but with distinct shapes when we need to override. Keep track of transitions leading to such nodes. This is much closer to JSC Structures but still lets us handle deep proto-chain hits with only two shape tests instead of N for N-deep.

Trick in this approach is to avoid sharing the wrong "duplicate node".

/be
I just want to throw another thought into the mix. In JSC, Structure is the analogue of scope+shape together. As you mutate an object, its Structure pointer changes. This has the advantage that a structure guard is one fewer load than our shape guards. The disadvantage is that things like purging a prototype chain by generating new shapes doesn't work--you'd have to clone structures to achieve something like that. There are probably other pluses and minuses--I haven't thought about it deeply.
I'm suggesting exactly the "clone the sprop [structure]" thing instead of making a shape override somewhere else, in comment 1. Fewer places to look, no flag bits to set and check. All you need is some duplicate-except-for-the-shape tolerance in the tree, and defense against bad sharing -- plus maybe transition memoizing for good sharing.

Right now objects divering into "own shape" land never reconverge, even if they all are being mutated in the same way, by the same code (but not predictably). So adding more predicition by memoizing transitions between states could help. That is what v8 does, and IIRC JSC has some placeholder code to do likewise (not sure how fleshed out -- haven't read in a while).

/be
Ah, I saw the reference to cloning and transitions in comment 1 but didn't get the significance. I think the JSC memoized transitions thing is in place now--there was a bunch of code for it last time I read, but I only read the details relating to the simplest uses.
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a5
Blocks: 564618
No longer blocks: 564618
Depends on: 566076
This is a big first step. Smaller patches to follow. Needs testing, builds both JS_THREADSAFE and not.

/be
Comment on attachment 445512 [details] [diff] [review]
fatten object with title, more fslots, ops (not yet used or set), flags

(Apologies if the line breaks in this comment are bad.)


> -const uint32 JS_INITIAL_NSLOTS = 5;
> +const uint32 JS_INITIAL_NSLOTS =
> +#ifdef JS_THREADSAFE
> +    6
> +#else
> +    10
> +#endif
> +    ;

Where do 6 and 10 come from?


> -    jsuword     classword;                  /* JSClass ptr | bits, see above 
*/
> +    JSClass     *clasp;                     /* JSClass ptr | bits, see above 
*/

Remove the "| bits, see above" ?


> +    JSClass *getClass() const { return clasp; }

Is that valid style?  I'm not complaining, I want to know if I can start
doing it :) 


> +JSObject::init(JSClass *aclasp, JSObject *proto, JSObject *parent, jsval priv
ateSlotValue,
> +               JSContext *cx)
> +{
> +    JS_STATIC_ASSERT(JSSLOT_PRIVATE + 3 <= JS_INITIAL_NSLOTS);
> +    JS_ASSERT_IF(clasp->flags & JSCLASS_HAS_PRIVATE,
> +                 (privateSlotValue & jsval(1)) == jsval(0));
> +
> +    clasp = aclasp;
> +    JS_ASSERT(!isDelegate());
> +    JS_ASSERT(!isSystem());
> +
> +    setProto(proto);
> +    setParent(parent);
> +    fslots[JSSLOT_PRIVATE] = privateSlotValue;
> +    fslots[JSSLOT_PRIVATE + 1] = JSVAL_VOID;
> +    fslots[JSSLOT_PRIVATE + 2] = JSVAL_VOID;
> +    dslots = NULL;
> +#ifdef JS_THREADSAFE
> +    js_InitTitle(cx, &title);
> +#endif
> +}

Do the extra fslots need to be initialized?  Also, 'cx' is often the first 
argument, is it worth making it so here?  (Likewise with defaultPrivate()?)


In jstracer.cpp, there's now no need to mask out the flag bits on the clasp 
when guarding on the class, which I'm sure you're aware of -- search for
JSSLOT_CLASS_MASK_BITS (which is a badly named constant, BTW, it has nothing
to do with slots).  Actually, the case in unbox_jsval() should probably just
call guardClass().  Any change here will conflict with my guardClass()
reworking (bug 565251) but not badly.
(In reply to comment #8)
> > -const uint32 JS_INITIAL_NSLOTS = 5;
> > +const uint32 JS_INITIAL_NSLOTS =
> > +#ifdef JS_THREADSAFE
> > +    6
> > +#else
> > +    10
> > +#endif
> > +    ;
> 
> Where do 6 and 10 come from?

This is temporary hacking to keep the object size 16 words. It'll get better as the patch queue progresses.

> > -    jsuword     classword;                  /* JSClass ptr | bits, see above 
> */
> > +    JSClass     *clasp;                     /* JSClass ptr | bits, see above 
> */
> 
> Remove the "| bits, see above" ?

Thanks.

> > +    JSClass *getClass() const { return clasp; }
> 
> Is that valid style?  I'm not complaining, I want to know if I can start
> doing it :) 

Yes, one-liners have always been ok if simple predicates or return-expr bodies.

> > +JSObject::init(JSClass *aclasp, JSObject *proto, JSObject *parent, jsval priv
> ateSlotValue,
> > +               JSContext *cx)
> > +{
> Do the extra fslots need to be initialized?

See bug 566076 -- we'll try to get that in sooner and rebase.

>  Also, 'cx' is often the first 
> argument, is it worth making it so here?  (Likewise with defaultPrivate()?)

We put cx last for infallible functions/methods. Also, this too is temporary until titles move out of native objects altogether, into non-native MT proxies.

> In jstracer.cpp, there's now no need to mask out the flag bits on the clasp 
> when guarding on the class, which I'm sure you're aware of -- search for
> JSSLOT_CLASS_MASK_BITS (which is a badly named constant, BTW, it has nothing
> to do with slots).

Yes, wanted to get to that last night but ran out of time, attached the patch as backup and for early review/testing help. Thanks,

/be
REFTEST INFO | Result summary:
REFTEST INFO | Successful: 54020 (54020 pass, 0 load only)
REFTEST INFO | Unexpected: 0 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 unexpected fixed asserts, 0 failed load, 0 exception)
REFTEST INFO | Known problems: 924 (91 known fail, 0 known asserts, 658 random, 175 skipped)
REFTEST INFO | Total canvas count = 0
REFTEST INFO | Quitting...
REFTEST INFO | runreftest.py | Running tests: end.

/be
> Yes, one-liners have always been ok if simple predicates or return-expr bodies.

This style rule applies to inline-defined methods in classes; inline functions not in classes are rarely one-liners, so they get the usual multiline style even if.

/be
Nick, anyone else able: feel free to test. Have to do stuff around the house today but I'll get back to this and do more stages tonight.

/be
Attachment #445512 - Attachment is obsolete: true
Comment on attachment 445559 [details] [diff] [review]
stage 1: fatten object with title, etc., and no more classword bit-twiddling

Busted, will fix.

/be
Attachment #445559 - Attachment is obsolete: true
Ugly, have to fix the places that know dslots is the one-stop-shopping-center for Call objects, arguments objects, and flat closures...

/be
(In reply to comment #14)
> Ugly, have to fix the places that know dslots is the one-stop-shopping-center
> for Call objects, arguments objects, and flat closures...

I have a patch awaiting feedback from you in bug 564094 that addresses some of that, and I was going to finish encapsulating the rest of the dslots accesses afterwards.

Bug 558265 is also relevant, though nothing has been done on it yet.
Nick, you were indeed ahead of the curve. We married our JS_INITIAL_NSLOTS value with abstraction-breaking code, in a bunch of places. As noted, long ago we split obj->slots in two and had to abstract, but those abstractions fell over time and in haste to optimization.

I've got things almost working now. I hope the win of Call objects using fslots to save the args and vars on deactivation of the Call's stack frame wins over the optimization bz did to read callobj->dslots in GetFromClosure, in jstracer.cpp. Will need help measuring to get this perf regression test-space covered quickly.

/be
This is bigger than I wanted, but going back and splitting it up would require temporizing (e.g. wasting fslots for call, block, arguments, and flat closure objects). Instead I marched through the code and the tests. Reencapsulating should be easier now. The only dslots-only assumptions I can see are for dense arrays. Rerunning in-browser jsreftests now, shell jsreftests & trace-tests pass.

/be
REFTEST INFO | Result summary:
REFTEST INFO | Successful: 54020 (54020 pass, 0 load only)
REFTEST INFO | Unexpected: 0 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 unexpected fixed asserts, 0 failed load, 0 exception)
REFTEST INFO | Known problems: 924 (91 known fail, 0 known asserts, 658 random, 175 skipped)
REFTEST INFO | Total canvas count = 0
REFTEST INFO | Quitting...

/be
Brendan, can I land my guardClass() changes (bug 565251), or would you prefer I wait?  I think it would cause you two small conflicts, one of which you already have written a comment about.
(In reply to comment #19)
> Brendan, can I land my guardClass() changes (bug 565251), or would you prefer I
> wait?  I think it would cause you two small conflicts, one of which you already
> have written a comment about.

Sure, land any time -- but I commented the place in unbox_jsval where I did not call guardClass. That place really does want to guard that the class is or is not Function, based on what the recorder saw. So it did not seem like your patch (if I remember it correctly) would change that site.

/be
(In reply to comment #20)
> 
> Sure, land any time -- but I commented the place in unbox_jsval where I did not
> call guardClass. That place really does want to guard that the class is or is
> not Function, based on what the recorder saw. So it did not seem like your
> patch (if I remember it correctly) would change that site.

That's easy to handle, there's a "if (isXYZ()) guardClass(...) else guardNotClass(...)" pattern that is used elsewhere.  I'll update the patch on bug 565251.
(In reply to comment #21)
> (In reply to comment #20)
> > 
> > Sure, land any time -- but I commented the place in unbox_jsval where I did not
> > call guardClass. That place really does want to guard that the class is or is
> > not Function, based on what the recorder saw. So it did not seem like your
> > patch (if I remember it correctly) would change that site.
> 
> That's easy to handle, there's a "if (isXYZ()) guardClass(...) else
> guardNotClass(...)" pattern that is used elsewhere.

Seems like more code that what's in this bug's stage 1 patch, now that the class pointer has no tag bits. Is it better for conditionally live guards to write the if-else that way in 565251?

/be
Can't remove JSScope::object without moving a bunch of stuff, starting with most of the flags, first.

/be
This one is wafer-thin.

/be
I think the `inline` keyword is worth keeping on those JSObject method declarations: if you #include "jsobj.h" but not "jsobjinlines.h" and try to use an inline, the `inline` declaration means you get a compile-time error instead of a link-time one.
(In reply to comment #34)
> I think the `inline` keyword is worth keeping on those JSObject method
> declarations: if you #include "jsobj.h" but not "jsobjinlines.h" and try to use
> an inline, the `inline` declaration means you get a compile-time error instead
> of a link-time one.

They cause warnings for all includers of jsobj.h who do not include jsobjinlines.h though. That seems worse. Including jsobjinlines.h in every place we include jsobj.h is over-including and it defeats the purpose of separating jsobjinlines.h.

Errors are errors, you get one from some part of the toolchain. But we have zero warning tolerance.

/be
(In reply to comment #35)
> They cause warnings for all includers of jsobj.h who do not include
> jsobjinlines.h though.

I don't think that is true in general; I think what you are seeing is an inline being used in a different inline, both in headers, which causes a warning even if neither inline is used elsewhere in the the translation unit.  In particular, I have seen this problem with an inline in jsscope.h using a jsobj.h inline which means that any includer of jsscope.h must also include jsobjinlines.h.  Perhaps this could be fixed by shuffling some definitions around.
Ooh, that would explain it -- I'm moving stuff from jsscopeinlines.h over to jsobjinlines.h. When the dust settles, I'll be sure to put back the inlines at the bottom of JSObject. Thanks,

/be
blocking2.0: --- → beta1+
Blocks: 569422
Blocks: 570435
blocking2.0: beta1+ → beta2+
Depends on: 535416
Depends on: 571789
bug558451-fatten-object
bug558451-redo-seal
bug558451-redo-brand
bug558451-redo-indexed
bug558451-move-own-shape-methods
bug558451-scope-hash-base
bug558451-scope-hash-ptr
bug558451-ops-move
bug558451-empty-scope-move
bug558451-rm-scope-object
bug558451-redo-freeslot
bug558451-move-own-shape-member
bug558451-merge-scope-and-sprop

Yet to come: remove-proptree-from-rt, remove-JSObjectMap-and-JSScope, shrink-JSObject, and misc-cleanup patches. Probably 16 total when all is done. I will attach individually when ready for review.

/be
Attachment #448924 - Attachment is obsolete: true
Attachment #448925 - Attachment is obsolete: true
Attachment #448926 - Attachment is obsolete: true
Attachment #448927 - Attachment is obsolete: true
Attachment #448928 - Attachment is obsolete: true
All trace-tests pass (six with some jitstats changes I still need to investigate but they improved ;-).

Among the js/src/tests suite, these fail:

js1_5/GC/regress-203278-2.js
js1_5/extensions/regress-365527.js

The first is because JSObject is temporarily too fat. The second needs more investigating.

/be
Depends on: 367425
blocking2.0: beta2+ → betaN+
Target Milestone: mozilla1.9.3a5 → mozilla2.0b3
This is close, modulo cleanups (most of which I should defer to followup patches in post-landing bugs). Sorry about the early patches, they are hard to subtract but should not get in the way.

/be
Will be jamming on this today as I'm able. With tests and tryserver joy I will attach the whole mq for review, today or tomorrow.

/be
Attachment #445494 - Attachment is obsolete: true
Attachment #455591 - Attachment is obsolete: true
Blocks: 583275
Attached patch backup hg export -r qbase:qtip (obsolete) — Splinter Review
Getting close, will try to wrap this up tonight and attach patches for review by tomorrow a.m.

There will be followup bugs for cleanups and improvements.

/be
Attachment #460418 - Attachment is obsolete: true
Blocks: 554109
Attachment #463032 - Attachment is obsolete: true
Attachment #464266 - Attachment is obsolete: true
Fixes a bad bug in JSObject::clear.

/be
Attachment #464683 - Attachment is obsolete: true
Will split up with notes for review today, modulo tryserver green.

/be
Attachment #465078 - Attachment is obsolete: true
Argh. I had hoped to eliminate JSObject::freeslot by making it a method, but that proved hard given the several built-in classes that allocate instance-varying numbers of reserved slots:

1. Functions that are flat closures -- these are referenced by script and their reserved slot count varies with the number of upvars.

2. Call objects -- these do not escape to script but eval("var haha") can add ad-hoc properties to them after their instance-varying slots, which vary with number of args and vars.

3. Arguments objects -- of course exposed to script, and their reserved slot count varies with the number of actual arguments.

Block, DeclEnv, and With objects are cool, they neither escape to script nor can grow ad-hoc properties (With of course have no varying reserved slots count).

So I kept freeslot, but forgot that the same problem (or another facet of it) still applies to shape-based first-slot prediction.

In the queued patch named bug558451-move-freeslot I changed the js_AddProperty builtin's AddPropertyHelper to assert that first predicted slot is equal to obj->freeslot, and of course tests such as this one (now added to trace-tests in that queued patch) violate this property:

(function(x) {
    function f() { return 22; }
    var g = function () { return x; }
    dis(g);
    var a = [f, f, f, f, g];
    for each (var v in a)
        v.adhoc = 42;   // Don't assertbotch in jsbuiltins.cpp setting g.adhoc
})(33);

The flat closure named g has an upvar, claiming the first dslot. So attempting to add g.adhoc matches the empty shape for functions but mispredicts 3, not 4, as the first free slot for adhoc.

I'd like to make the effort to fix the three hard cases above, instead of giving up on first slot being strictly determined by empty shape and first-slot-added program point. More in a bit.

/be
(In reply to comment #54)
> 1. Functions that are flat closures -- these are referenced by script and their
> reserved slot count varies with the number of upvars.
> 
> 2. Call objects -- these do not escape to script but eval("var haha") can add
> ad-hoc properties to them after their instance-varying slots, which vary with
> number of args and vars.
> 
> 3. Arguments objects -- of course exposed to script, and their reserved slot
> count varies with the number of actual arguments.

What about using tail slots for these cases or storing them in slots before before dslots[-1]? Alternatively the corresponding variables can be stored in a separately allocated array not to mess slots allocation at all. The drawback is that it would require to bump the size of object to accommodate the extra pointer but given the plans to allocate objects of different sizes that may not be that bad.
Corrected test from comment 54:

(function(x) {
    function f1() { return 1; }
    function f2() { return 2; }
    function f3() { return 3; }
    function f4() { return 4; }
    var g = function () { return x; }
    var a = [f1, f2, f3, f4, g];
    for each (var v in a)
        v.adhoc = 42;   // Don't assertbotch in jsbuiltins.cpp setting g.adhoc
})(33);

I've added this to trace-tests in the bug558451-move-freeslot, two patches prior to the big merge-scope-and-sprop patch -- and thereby found that the latter patch had bad bugs in both JSOP_SETPROP (etc.)'s fast path logic and in the property cache fill code. These bugs entailed not keeping up with the change to have an empty sprop (scope) at the top of the property lineage -- there were naked if (!sprop->parent) tests wrongly conditioning empty-scope logic.

Fixing that and moving forward to keep the test failing until I can enforce the "no varying number of instance-reserved slots for objects that might grow ad-hoc properties" rule.

Re: comment 55: we don't want dslots[-1], bhackett has separated that into a capacity field so we can point dslots at fslots when fslots are enough. Plus we want to "right-size" objects so fslots will (with good predictability based on allocation site) be enough. So that leaves separate storage for the reserved slots if they can't be *real* property slots.

Case-by-case:

A. Arguments object reserved slots are needed once the frame pops, and we do not want a fallible allocation then, so we pre-allocate with the arguments objects. Right now we pre-allocate dslots, but it costs no more to pre-allocate separate storage, and we can hang onto it using a reserved slot with a bit of re-work.

B. Call objects need dslots but these can be real, property-mapped slots, with the call object gaining a non-empty shape at construction time, via a compiler-constructed property tree path. This requires unshared accessor properties (i.e., ones with slots) to be traced but that restriction can be lifted in at least the case of Call objects. The compiler-constructed property tree path can include upvars and replace the fun->u.i.names singleton/array/hashmap. I have a patch for this already set aside, which I'll revive.

C. Flat closures need separate storage, which can be allocated and referenced from a reserved slot (functions have two, for embedding purposes only where the embedding creates the native function, so flat closures can use these otherwise). Again as with (A) this allocation is today stored in dslots, so there is no net increase in allocation cost.

/be
(In reply to comment #56)
> These bugs entailed not keeping up with the
> change to have an empty sprop (scope) at the top of the property lineage --
> there were naked if (!sprop->parent) tests wrongly conditioning empty-scope
> logic.

A grep for 'sprop->parent' in if conditions shows no more badness lingering from the old property tree structure, prior to merge-scope-and-sprop, where the oldest node in the lineage was the first property instead of the empty shape/scope. I'll add a C++ iterator to abstract away from the new representation for loops that want to go over real (non-empty-scope) sprops/shapes.

/be
Attachment #465252 - Attachment is obsolete: true
With 23 patches at a late hour, I thought I'd try a gzipped tarball. Jason, if you want individual attachments I will do the work tomorrow. Happy to attach them as needed, or all up front if you prefer.

The series names should help explain things:

 0 A bug558451-fatten-object
 1 A bug558451-redo-seal
 2 A bug558451-redo-brand
 3 A bug558451-redo-indexed
 4 A bug558451-move-own-shape-methods
 5 A bug558451-scope-hash-base
 6 A bug558451-scope-hash-ptr
 7 A bug558451-move-ops
 8 A bug558451-move-empty-scope
 9 A bug558451-rm-scope-object
10 A bug558451-move-freeslot
11 A bug558451-move-own-shape-member
12 A bug558451-merge-scope-and-sprop
13 A bug558451-rm-empty-shapes
14 A bug558451-rm-js_GetMutableScope
15 A bug558451-rename-sprop
16 A bug558451-abstract-and-constipate-shape
17 A bug558451-args-and-call-cleanup
18 A bug558451-use-shapes-for-fun-u.i.names
19 A bug558451-no-call-object-dslots
20 A bug558451-no-args-element-dslots
21 A bug558451-no-flat-closure-upvar-dslots
22 A bug558451-recycle-deleted-slots

I took some wrong turns (abstract-and-constipate overdid the const-ipation) but corrected course in later patches. I have a few TODO items unaddressed:

[x] fix the variable-per-instance first expando slot number problem
 => followup bug to prevent this from recurring (e.g., bound function objects)
[n] common the code expanded in three places before the obj->extend(...) calls
 => do this in a followup bug
[x] SHAPE_HAS_VALID_SLOT still used? it's defined!
[ ] Grammar in comment in ensureClassReservedSlotsForEmptyObject
[ ] js_GetCallVarChecked needed? If so, redo without resolve

Followup fodder above adds to the older list of known-needed followup bugs:

1. Remove JSScope, just use js::Shape.
2. Rename JSEmptyScope to js::EmptyShape (bhackett's plan for multiple fslots per class)
3. Remodularize: jspropertytree.cpp/.h is obsolete, jsscope.cpp/.h -> jsshape...
4. Meta: figure out despecialization strategies to avoid ownShape proliferation
4a. shadowing
4b. branding
4c. ...

I am no doubt forgetting something.

/be
Attachment #466924 - Attachment is obsolete: true
Attachment #466964 - Flags: review?(jorendorff)
Big followup bug not in last comment: remove JS_THREADSAFE macrology and inlines and make shape heap per-compartment, along with the six well-known empty scopes. This could happen soon but I don't want to break the world too much -- with the default compartment still collecting stray gc-things, it seems better to keep the runtime-wide heap/empty-scope-set for now.

/be
But (tired, forgot to add to comment 60) the real win of removing JS_THREADSAFE is trimming JSObject down by four words. The current bloat for JSTitle embedded as a member may hurt perf noticeably compared to doing without this overhead.

But we still win by eliminating JSScope, of course -- just not as much as we will win once bug 566951 is fixed and JSObject is free of #ifdef JS_THREADSAFE bloat.

/be
Blocks: 535629
blocking2.0: betaN+ → beta5+
22 A bug558451-recycle-deleted-slots was bogus, a better patch to recycle slots is now where it belongs, in bug 535629.

21 A bug558451-no-flat-closure-upvar-dslots has a dumb bug -- forgot to mark the upvars in their new storage. I'll attach interdiff or do something better like maybe attach all 22 patches in a bit.

/be
Blocks: 367425, 566076
No longer depends on: 367425, 566076
Review for patch 0 (bug558451-fatten-object):

In jsapi.cpp, JS_CloneFunctionObject:
>+            Value *vp = (slot < JS_INITIAL_NSLOTS)
>+                        ? &clone->fslots[slot]
>+                        : &clone->dslots[slot - JS_INITIAL_NSLOTS];

The RHS here can be written &clone->getSlotRef(slot).

In jsfun.cpp:
> inline static void
> CopyValuesToCallObject(JSObject *callobj, uintN nargs, Value *argv, uintN nvars, Value *slots)
> {
>-    memcpy(callobj->dslots, argv, nargs * sizeof(Value));
>-    memcpy(callobj->dslots + nargs, slots, nvars * sizeof(Value));
      /* Copy however many args fit into fslots. */
>+    uintN freeslot = JSSLOT_PRIVATE + CALL_CLASS_FIXED_RESERVED_SLOTS + 1;
>+    Value *vp = &callobj->fslots[freeslot];
>+    uintN len = JS_MIN(nargs, JS_INITIAL_NSLOTS - freeslot);
>+
>+    memcpy(vp, argv, len * sizeof(Value));
>+    vp += len;
>+
>+    nargs -= len;
>+    if (nargs) {
	  /* Copy any remaining args into dslots. */
>+        vp = callobj->dslots;
>+        memcpy(vp, argv + len, nargs * sizeof(Value));
>+        vp += nargs;
>+    } else {
	  /* Copy however many vars fit into the remaining fslots. */
>+        freeslot += len;
>+        len = JS_MIN(nvars, JS_INITIAL_NSLOTS - freeslot);
>+        memcpy(vp, slots, len * sizeof(Value));
>+        slots += len;
>+        nvars -= len;
>+        vp = callobj->dslots;
>+    }
>+
      /* Copy any remaining vars into dslots. */
>+    memcpy(vp, slots, nvars * sizeof(Value));
> }

I found this surprisingly hard to read for such a simple task. Please add the
four one-liner comments I sprinkled in above.

In jsfun.cpp, CallPropertyOp:
>     if (kind == JSCPK_UPVAR) {
>         JSObject *callee = &obj->getSlot(JSSLOT_CALLEE).toObject();
> 
> #ifdef DEBUG
>+        JS_ASSERT(!setter);
>+

setter is true here if called from SetFlatUpvar.

In jslock.cpp, js_LockObj:
>     for (;;) {
[...]
> 
>-        js_LockTitle(cx, title);
>+        js_LockTitle(cx, &obj->title);
> 
>         /* If obj still has this scope, we're done. */
>         if (scope == obj->scope())
>             return;
> 
>         /* Lost a race with a mutator; retry with obj's new scope. */
>-        js_UnlockTitle(cx, title);
>+        js_UnlockTitle(cx, &obj->title);
>     }

The loop is no longer necessary, right?

In jsobj.h, class JSObject:
>+    bool isDelegate() const { return flags & DELEGATE; }
...
>+    bool isSystem() const   { return flags & SYSTEM; }

These need `return (flags & DELEGATE) != 0;` or some such circumlocution to avoid an MSVC warning.

>+    inline void finish(JSContext *cx);

Nit: "Finish" can mean either "complete" or "tear down". Can we call it "destroy"?

In jsobjinlines.h:
> inline bool
>+JSObject:: hasProperty(JSContext *cx, jsid id, bool *foundp)

Nit: Stray space before hasProperty.

In jspropertycache.cpp, JSPropertyCache::fill:
>              * This is thread-safe even though obj is not locked. Only the
>-             * DELEGATE bit of obj->classword can change at runtime, given that
>+             * DELEGATE bit of obj->flags can change at runtime, given that
>              * obj is native; and the bit is only set, never cleared. And on

I don't think this comment is correct anymore, with or without the patch.

This is more than a nit--the change to JSObject::flags is unsafe, I think,
until gal's multi-thread proxies are implemented. The comment should say so.

In jstypedarray.cpp:
>     makeFastWithPrivate(JSContext *cx, JSObject *obj, ThisTypeArray *tarray)
>     {
>         JS_ASSERT(obj->getClass() == slowClass());
>         obj->setPrivate(tarray);
>+
>+        // now set the class and map to make this into a fast typed array
>         obj->clasp = fastClass();
>         obj->map = const_cast<JSObjectMap *>(&JSObjectMap::sharedNonNative);
>     }

That added comment doesn't seem necessary, but your call.

r=me with the comments addressed.
> In jslock.cpp, js_LockObj:
>[...]
> The loop is no longer necessary, right?

Never mind -- the next patch in the queue fixes it.
Review for patch 1 (redo-seal):

No more JS_DEBUG_TITLE_LOCKS? I thought there was still a pretty good chance
gal would reuse all this jslock.cpp code, and this code might come in handy.

r=me.
Blocks: 492844
Blocks: 492845
Blocks: 492849
Jason caught one in jsfun.cpp:CallPropertyOp by inspection, which the jsreftests found.

Another change here is unifying so obj->shape() just loads obj->objShape, which requires syncing that member from obj->map->shape. That's the new patch on top of the stack. Fixing that turned up skeletons in my closet: some trace-tests I had patched to match too-good-to-be-true jitstats. I reverted the tests and now everything passes as before.

/be
Attachment #466964 - Attachment is obsolete: true
Attachment #466964 - Flags: review?(jorendorff)
Review for patch 2 (redo-brand):

In jsobj.h, struct JSObject:
>+    /*
>+     * Test whether this scope may be branded due to method calls,

"whether this object"...

>+    /*
>+     * Read barrier to clone a joined function object stored as a method.
>+     * Defined in jsscopeinlines.h, but not declared inline per standard style

"Defined in jsobjinlines.h,"...

>+     * Write barrier to check for a method value change. Defined inline below
>+     * after methodReadBarrier. Two flavors to handle JSOP_*GVAR, which deals

This is also defined in jsobjinlines.h, not "below".

(removed from class JSScope:)
>-    bool brand(JSContext *cx, uint32 slot, const js::Value &) {
>-        JS_ASSERT(!generic());
>-        JS_ASSERT(!branded());
>-        generateOwnShape(cx);
>-        if (js_IsPropertyCacheDisabled(cx))  // check for rt->shapeGen overflow
>-            return false;
>-        flags |= BRANDED;
>-        return true;
>-    }
(added in jsobjinlines.h:)
>+inline bool
>+JSObject::brand(JSContext *cx, uint32 slot, js::Value v)
>+{
>+    JS_ASSERT(!generic());
>+    JS_ASSERT(!branded());
>+    if (isNative()) {
>+        scope()->generateOwnShape(cx);
>+        if (js_IsPropertyCacheDisabled(cx))  // check for rt->shapeGen overflow
>+            return false;
>+    }
>+    flags |= BRANDED;
>+    return true;
>+}

You can assert isNative() instead of checking it here. It's only called in one
place (property cache fill) and there we know the object is native, right?

r=me with those changes.
Review for patch 3 (redo-indexed): r=me.
Review for patch 4 (move-own-shape-methods): r=me.
Review for patch 5 (scope-hash-base): r=me.

In jsscope.h, JSScope::search:
>-    return searchTable(id, adding);
>+    return js::PropertyHash::search(id, adding);

Heh. If this were a standalone patch I would be making faces right now.
I'll process review comments later tonight. Thanks for those, jorendorff!

/be
Attachment #467195 - Attachment is obsolete: true
Oh, and I'm gonna process review comments (where still relevant given later mq'ed patches) by pushing new patches. Changing low in the stack is painful (patch fail -> .rej files; hand-editing .hg/patches/bug55845* fun; etc.).

/be
Review for patch 6 (scope-hash-ptr):

In jsscope.cpp, PropertyHash::init:
>-    JS_ASSERT(!table);

That assertion should stay, I think.

r=me with that (or an explanation).

Followup fodder: The way JSScope still freely munges js::PropertyHash's private state is awfully ugly. Can't PropertyHash be a js::HashSet? It would need a slightly custom HashPolicy, like so:

    struct PropertyHasher {
        typedef jsid Lookup;
        static HashNumber hash(Lookup id) { return HashNumber(id); }
        static bool match(js::Shape *shape, Lookup id) { return shape->id == id; }
    };

    typedef HashSet<js::Shape *, PropertyHasher, SystemAllocPolicy> PropertyTable;

The resulting class uses more table memory than the current code, but I don't think it's a lot of memory in the scheme of things (given the load factor and the size of the pointed-to Shapes, I think the table itself isn't the main space cost here) and we might even gain some speed in exchange for that space.
It's possible jshashtable.h while chewing more memory would lookup faster, due to some inlining enabled by templating. It's far from obvious, though.

We could work harder on jshashtable.h by delegating the collided, removed, and free sentinel predicates to the client. That could easily undo some of the inlining wins, though.

Another idea is to templatize PropertyTable a la HashMap without trying to force unification, get some data on this alternative.

IOW I don't think just "space is cheap, let's use jshashtable" vs. "stick with the old C-ish code" should be the set of trade-offs we spend time evaluating. This might later feed back more fruitfully into jshashtable.h without an all-or-nothing bet.

Turns out JSC has a custom ggaren hashtable in its Structures, which Oliver wants to replace with a WTF standard thingie. But that too is low priority. Same story all over!

/be
> Another idea is to templatize PropertyTable a la HashMap without trying to

I meant "inline" rather than "templatize", since there's only one key type. Also it's reall a HashSet, the key is the value.

Anyway, I thought about using jshashtable.h, since I like it a lot. But it's not time, and some care is needed. Just wanted to record that this was given thought.

/be
Review for patch 7 (move-ops):

In jsscope.cpp:
> JSScope *
> JSScope::create(JSContext *cx, Class *clasp, JSObject *obj, uint32 shape)
> {
>-    JS_ASSERT(obj);
>-
>     JSScope *scope = cx->create<JSScope>(obj);
>     if (!scope)
>         return NULL;
> 
>     scope->freeslot = JSSLOT_START(clasp);
>     scope->flags = cx->runtime->gcRegenShapesScopeFlag;
>     scope->initMinimal(cx, shape);

Why remove this assertion?

r=me with that.
(In reply to comment #76)

Probably I just removed non-null assertions since we generally don't bother. They are no more informative than a crash dereferencing null (we think; are we missing some benefit?).

/be
They are redundant if we were going to reference that memory soon anyway, producing the crash you're talking about.

But in this case obj is just stored in the new scope, and a null object is ok in that field, under other circumstances. We may never crash, just exhibit really weird wrong behavior. In cases like that, it's worth asserting.
Review for patch 8 (bug558451-move-empty-scope): r=me.
Review for patch 9 (rm-scope-object):

In jsobj.cpp, js_DefineNativeProperty:
>-        sprop = scope->putProperty(cx, id, getter, setter, SPROP_INVALID_SLOT,
>-                                   attrs, flags, shortid);
>+        sprop = obj->putProperty(cx, id, getter, setter, SPROP_INVALID_SLOT,
>+                                 attrs, flags, shortid);

Can we change the name of this method, while we're at it?

    getProperty === ES5 [[GetProperty]]   (at least very very close)
    putProperty !== ES5 [[Put]]           (totally unrelated)

Any new name is fine with me; addOrChangeProperty would be a net improvement.

It'll be nice to get all this code in one place and reread it.

In jsobj.cpp, js_SetPropertyHelper:
>-         * Set scope for use below.  It was locked by js_LookupProperty, and
>-         * we know pobj owns it (i.e., scope->object == pobj).  Therefore we
>+         * Set scope for use below. It was locked by js_LookupProperty, and we
>+         * know pobj owns it (i.e., !scope->isSharedEmpty()). Therefore we

This comment still refers to scope as locked.

In jsobj.h, struct JSObject:
>+    /* Extend this scope to have sprop as its last-added property. */
>+    inline void extend(JSContext *cx, JSScopeProperty *sprop, bool isDefinitelyAtom = false);

"Extend this object"...

r=me with at least the comments fixed.
Partial review for patch 10 (move-freeslot):

In jsscope.cpp:
> #define INIT_EMPTY_SCOPE_WITH_CLASS(Name,NAME,clasp)                          \
>-    INIT_EMPTY_SCOPE_WITH_FREESLOT(Name, NAME, clasp, JSSLOT_FREE(clasp))
>+    INIT_EMPTY_SCOPE_WITH_FREESLOT(Name, NAME, clasp)
> 
>-#define INIT_EMPTY_SCOPE_WITH_FREESLOT(Name,NAME,clasp,slot)                  \
>+#define INIT_EMPTY_SCOPE_WITH_FREESLOT(Name,NAME,clasp)                       \
>     SCOPE(Name) = cx->create<JSEmptyScope>(cx, clasp);                        \
>     if (!SCOPE(Name))                                                         \
>         return false;                                                         \
>     JS_ASSERT(SCOPE(Name)->shape == JSScope::EMPTY_##NAME##_SHAPE);           \
>     JS_ASSERT(SCOPE(Name)->nrefs == 2);                                       \
>     SCOPE(Name)->nrefs = 1;                                                   \
>-    SCOPE(Name)->freeslot = slot

INIT_EMPTY_SCOPE_WITH_FREESLOT is a misnomer now. Move its code into
INIT_EMPTY_SCOPE_WITH_CLASS and use that below:

    INIT_EMPTY_SCOPE_WITH_CLASS(Arguments, ARGUMENTS, CLASP(Arguments));

I'm a little over halfway done with this one but I have to go get an X-ray.
(In reply to comment #63)
> Review for patch 0 (bug558451-fatten-object):
> 
> In jsapi.cpp, JS_CloneFunctionObject:
> >+            Value *vp = (slot < JS_INITIAL_NSLOTS)
> >+                        ? &clone->fslots[slot]
> >+                        : &clone->dslots[slot - JS_INITIAL_NSLOTS];
> 
> The RHS here can be written &clone->getSlotRef(slot).

All better now in bug558451-no-flat-closure-upvar-dslots and even better in the patch I'm about to attach.

> I found this surprisingly hard to read for such a simple task.

Is it intrinsic complexity or something else (variable naming)?

> Please add the four one-liner comments I sprinkled in above.

Done -- thanks!

> In jsfun.cpp, CallPropertyOp:
> >     if (kind == JSCPK_UPVAR) {
> >         JSObject *callee = &obj->getSlot(JSSLOT_CALLEE).toObject();
> > 
> > #ifdef DEBUG
> >+        JS_ASSERT(!setter);
> >+
> 
> setter is true here if called from SetFlatUpvar.

Fixed in bug558451-no-flat-closure-upvar-dslots.

> In jsobj.h, class JSObject:
> >+    bool isDelegate() const { return flags & DELEGATE; }
> ...
> >+    bool isSystem() const   { return flags & SYSTEM; }
> 
> These need `return (flags & DELEGATE) != 0;` or some such circumlocution to
> avoid an MSVC warning.

MSVC is lame. Fixed.

> >+    inline void finish(JSContext *cx);
> 
> Nit: "Finish" can mean either "complete" or "tear down". Can we call it
> "destroy"?

SpiderMonkey uses finish both ways but with noun phrases after the verb to help if the context is not enough. "Destroy" means deallocate, but that's not what this method does, so I'm sticking with finish. Would like to use a dtor -- but that is for a followup bug on beautifying JSObject.

> In jspropertycache.cpp, JSPropertyCache::fill:
> >              * This is thread-safe even though obj is not locked. Only the
> >-             * DELEGATE bit of obj->classword can change at runtime, given that
> >+             * DELEGATE bit of obj->flags can change at runtime, given that
> >              * obj is native; and the bit is only set, never cleared. And on
> 
> I don't think this comment is correct anymore, with or without the patch.
> 
> This is more than a nit--the change to JSObject::flags is unsafe, I think,
> until gal's multi-thread proxies are implemented. The comment should say so.

Fixed, thanks.

> In jstypedarray.cpp:
> >     makeFastWithPrivate(JSContext *cx, JSObject *obj, ThisTypeArray *tarray)
> >     {
> >         JS_ASSERT(obj->getClass() == slowClass());
> >         obj->setPrivate(tarray);
> >+
> >+        // now set the class and map to make this into a fast typed array
> >         obj->clasp = fastClass();
> >         obj->map = const_cast<JSObjectMap *>(&JSObjectMap::sharedNonNative);
> >     }
> 
> That added comment doesn't seem necessary, but your call.

Fixed by reordering to match member order and leaving out redundant comments.

(In reply to comment #65)
> No more JS_DEBUG_TITLE_LOCKS? I thought there was still a pretty good chance
> gal would reuse all this jslock.cpp code, and this code might come in handy.

I ripped it out; Hg knows all and will tell if asked. Andreas may restore it but without DEBUG coverage it was bitrot bait. It wasn't all that useful in recent years, either, although you may be right that it will suddenly want to make a come-back.

(In reply to comment #67)

Fixed everything not commented on here, and fixed this one too:

> >+JSObject::brand(JSContext *cx, uint32 slot, js::Value v)
> >+{
> >+    JS_ASSERT(!generic());
> >+    JS_ASSERT(!branded());
> >+    if (isNative()) {
> >+        scope()->generateOwnShape(cx);
> >+        if (js_IsPropertyCacheDisabled(cx))  // check for rt->shapeGen overflow
> >+            return false;
> >+    }
> >+    flags |= BRANDED;
> >+    return true;
> >+}
> 
> You can assert isNative() instead of checking it here. It's only called in one
> place (property cache fill) and there we know the object is native, right?

Yes, thanks. And I changed unbrand to match: it asserts isNative(), requiring the JSOP_UNBRAND_THIS op's code in jsinterp.cpp to test obj->isNative(). See the patch I'm about to attach.

(In reply to comment #78)
> But in this case obj is just stored in the new scope, and a null object is ok
> in that field, under other circumstances. We may never crash, just exhibit
> really weird wrong behavior. In cases like that, it's worth asserting.

JSScope::create all gone, no opportunity or need for not-null assertion. But (followup fodder) Luke has convinced me that using C++ references to mean not-null wins. Duh, type-checking FTW. So we should have a bug to do that more.

(In reply to comment #80)
> In jsobj.cpp, js_DefineNativeProperty:
> >-        sprop = scope->putProperty(cx, id, getter, setter, SPROP_INVALID_SLOT,
> >-                                   attrs, flags, shortid);
> >+        sprop = obj->putProperty(cx, id, getter, setter, SPROP_INVALID_SLOT,
> >+                                 attrs, flags, shortid);
> 
> Can we change the name of this method, while we're at it?

Not now, but rest assured I have a plan. You and others (hi Waldo!) will like it. Meta-methods based on ES5 as JSObject methods, and differently-named Shape methods. The end-goal is to have !isNative() <=> isProxy(). Ok?

> It'll be nice to get all this code in one place and reread it.

Remodularization is coming, and before Fx4 to avoid maintenance branch hell. I will put up a wiki page and get consensus before filing bugs.

> In jsobj.cpp, js_SetPropertyHelper:
> >-         * Set scope for use below.  It was locked by js_LookupProperty, and
> >-         * we know pobj owns it (i.e., scope->object == pobj).  Therefore we
> >+         * Set scope for use below. It was locked by js_LookupProperty, and we
> >+         * know pobj owns it (i.e., !scope->isSharedEmpty()). Therefore we
> 
> This comment still refers to scope as locked.

I removed this comment. Less to remove in the followup bug to eliminate JSScope and any JSScope *scope locals! ;-)

Problem from comment 81 fixed -- thanks!

/be
Attached file updated per last comment (obsolete) —
Attachment #467250 - Attachment is obsolete: true
Attachment #467656 - Flags: review?(jorendorff)
Review for patch 10 (move-freeslot):

In jsbuiltins.cpp:
> static inline JSBool
> AddPropertyHelper(JSContext* cx, JSObject* obj, JSScopeProperty* sprop, bool isDefinitelyAtom)

We previously tolerated a mismatching sprop->slot, here and elsewhere, because
shape didn't cover freeslot, due to reserveSlots, right? And bug 535416 fixed
that?

In js_SetReservedSlot and again in js_DumpObject, comments and code about
freeslot being an underestimate were deleted, but in js_TraceObject they were
retained. What changed?

jsobj.cpp, js_PutBlockObject:
>      * Block objects should never be exposed to scripts. Thus the clone should
>      * not own the property map and rather always share it with the prototype
>+     * object.

The second sentence here isn't accurate (not for a while).

In jsscope.cpp:
>-     * Subtle dependency on objects that call js_EnsureReservedSlots either:
>+     * Subtle rule: objects that call JSObject::ensureReservedSlots either
>      * (a) never escaping anywhere an ad-hoc property could be set on them;
>      * (b) having at least JSSLOT_FREE(obj->getClass()) >= JS_INITIAL_NSLOTS.

This edit makes the comment ungrammatical. Changing escaping->escape and
having->have would fix it.

r=me with the comments fixed.
Review for patch 11 (move-own-shape-member): r=me.
> Luke has convinced me that using C++ references to mean
> not-null wins.

Almost everything Luke says is right, so I hesitate to say it... but I disagree very strongly on this.

The compiler does not actually prevent you from dereferencing a null pointer to make a null reference. It does not check statically and it does not emit a runtime check. So there is no real safety benefit to using references. There is an explicitness benefit, but without the compiler checking you, it's equivalent to a comment.

References screw up basic reasoning about code. Time was, I could assume that foo(*vp) was passing *vp by value. Now at best I can't assume that, and at worst I assume it anyway (because dammit that was an invariant in C and in C++ the way we used to use it, sue me) and I get bitten.

And there's the fact that the totally safe and harmless (nonnull -> nullable) conversion isn't implicit, so we get to do even more compiler appeasement passes.

And there's the fact that we could never use this technique consistently everywhere because there are a bunch of places where references can't be used.

And on and on -- luke knows the arguments, there are like 100 of them; non-const references are an abomination and using them outside the couple of situations where they're better than a sharp stick in the eye turns your codebase into a big bag of sharp sticks in the eye. toObject() makes me really sad.
(Looks like patch 11 breaks the tracer by changing the mechanics of shape lookups without touching js::TraceRecorder::shape_ins. Patch 12 restores everything to working order. Just a note, since we don't plan landing this in pieces anyway.)
(In reply to comment #84)
> Review for patch 10 (move-freeslot):
> 
> In jsbuiltins.cpp:
> > static inline JSBool
> > AddPropertyHelper(JSContext* cx, JSObject* obj, JSScopeProperty* sprop, bool isDefinitelyAtom)
> 
> We previously tolerated a mismatching sprop->slot, here and elsewhere, because
> shape didn't cover freeslot, due to reserveSlots, right? And bug 535416 fixed
> that?

Only if the variation in first property slot could be due to reserveSlots. But we still have the problem not fixed until patch 21 bug558451-no-flat-closure-upvar-dslots in this bug's series: some built-in objects have dslots reserved for a variable number of slots, without any class hook and without giving the object a different empty shape.

We don't want the overhead of creating (or caching/hash-consing) an empty shape for each possible starting freeslot. So I've moved such dslots uses to other memory that is not indexed on the slot number line.

I'll fix up comment bugs noted here. And yes, I left some things broken (and not tested by trace-test gaps) until later in the q. Keep going!

> In jsscope.cpp:
> >-     * Subtle dependency on objects that call js_EnsureReservedSlots either:
> >+     * Subtle rule: objects that call JSObject::ensureReservedSlots either
> >      * (a) never escaping anywhere an ad-hoc property could be set on them;
> >      * (b) having at least JSSLOT_FREE(obj->getClass()) >= JS_INITIAL_NSLOTS.
> 
> This edit makes the comment ungrammatical. Changing escaping->escape and
> having->have would fix it.

Yeah, I fixed the grammar and kept reverting the fix, and lost it ultimately. See my TODO list in comment 59:

[ ] Grammar in comment in ensureClassReservedSlotsForEmptyObject

/be
(In reply to comment #87)
> (Looks like patch 11 breaks the tracer by changing the mechanics of shape
> lookups without touching js::TraceRecorder::shape_ins. Patch 12 restores
> everything to working order. Just a note, since we don't plan landing this in
> pieces anyway.)

Yeah, patch 11 was wacky -- ownShape as 24-bit field makes loading via a 32-bit load kinda hard. I decided to plow ahead rather than sink more time on this, but also ended up swapping ownShape (now objShape, always valid) and freeslot. So until we can compute freeslot from lastProp->slot, we have at most 24 bits to index the next free slot.

I'm almost certainly missing an overflow check on this. Will get to it shortly.

/be
(In reply to comment #86)
> > Luke has convinced me that using C++ references to mean
> > not-null wins.
> 
> Almost everything Luke says is right, so I hesitate to say it... but I disagree
> very strongly on this.

Whoa, I was not endorsing references instead of pointers for in/out and out params. I think you and I have agreed on that in the past. Good news is that Luke told me you convinced him too! :-)

> The compiler does not actually prevent you from dereferencing a null pointer to
> make a null reference. It does not check statically and it does not emit a
> runtime check. So there is no real safety benefit to using references. There is
> an explicitness benefit, but without the compiler checking you, it's equivalent
> to a comment.

A bit more than that, although you're right there's no null defense at the limit. The type checker helps you "taint" your program, with a lighterweight and less evil way of controlling propagation (unary & instead of const_cast). This is what I found winning, after first reacting negatively (Luke heard me convert on IRC and was happy to see such flexibility in this old dog ;-).

> References screw up basic reasoning about code. Time was, I could assume that
> foo(*vp) was passing *vp by value. Now at best I can't assume that, and at
> worst I assume it anyway (because dammit that was an invariant in C and in C++
> the way we used to use it, sue me) and I get bitten.

This is where I believe everyone agrees who has spoken up. We really do want unary-& in callsites to make it clear something is being passed as an in/out or out parameter. Total agreement (from me and I believe Luke).

> And there's the fact that the totally safe and harmless (nonnull -> nullable)
> conversion isn't implicit, so we get to do even more compiler appeasement
> passes.

This was helpful in finding places where fatvals can distinguish ObjectOrNull from Object. I found a few more post-landing.

> And there's the fact that we could never use this technique consistently
> everywhere because there are a bunch of places where references can't be used.

This is a good point. References have "issues" even if we avoid them for out params.

Just the difference between initializing a reference and then assigning to the variable of reference type is confusing. I've seen seasoned programmers get it wrong. Using = for both operations is bad, and I recall some C++ stylists therefore arguing for constructor-style initialization -- but it's not mandatory and it looks weird to some (and you can forget the declarator name and make a useless call, no error).

> And on and on -- luke knows the arguments, there are like 100 of them;
> non-const references are an abomination and using them outside the couple of
> situations where they're better than a sharp stick in the eye turns your
> codebase into a big bag of sharp sticks in the eye.

Agree on non-const refs for params. Wonder if I'm just missing my eye by luck in this patch. Keep reading, you'll see some local non-const& uses.

> toObject() makes me really sad.

Again this should be separated from the out param issue. If you do that and we agree on that one, is toObject() still sad-making?

/be
(In reply to comment #90)
> This is where I believe everyone agrees who has spoken up. We really do want
> unary-& in callsites to make it clear something is being passed as an in/out or
> out parameter. Total agreement (from me and I believe Luke).

Total agreement.  Actually, in the time since Jason made the case against &-outparams on IRC a few months ago, I've been actively converting &-outparams to *-outparams when they neighbor code I touch.
Threw this together while poking at the next patch. (So far, I don't think I've found any bugs in the entire stack.)
Assignee: brendan → jorendorff
Attachment #467946 - Flags: review?(brendan)
Partial review for patch 12 (merge-scope-and-sprop):

In jsscope.h:
>  * JSScope uses multiplicative hashing, _a la_ jsdhash.[ch], but specialized

"JSPropertyTable uses"...

In jsscope.h, struct JSScope:
>-    inline JSScopeProperty **search(jsid id, bool adding);
>-
>     bool maybeHash(JSContext *cx);

Heh. It seems a little random, sometimes, what remained in JSScope and what was
moved! In fact... in one or two cases it seems like the wrong choice:

>+inline bool
>+JSScope::isSharedEmpty() const
>+{
>+    return ((JSScopeProperty *)this)->flags & JSScopeProperty::SHARED_EMPTY;
>+}

That cast reads like a signed confession ;-). If this predicate only
applies to JSScopeProperties, why not make it a member of JSScopeProperty?
entryCount() too.

>-    JSScope(JSObject *obj) : JSObjectMap(0), flags(obj ? SHARED_EMPTY : 0) {}
>+    JSScope(uint32 shape) : JSObjectMap(shape), table(NULL) {}

'explicit' on all one-argument constructors, please.

Once--in DOS days--I had a bug where my code said
  someFunction(3);
That was wrong. The function didn't take an int; it took an object of some class--it might as well have been JSScope--it was big and 3 was a totally meaningless argument to its constructor. But it did have a non-explicit constructor that took a single int argument, so C++ was implicitly converting 3 to that type for me. This was ridiculously hard to debug. Partly because the crash left me in some goofy VGA graphics mode from which my only recourse was to reboot, sure. But from then on I used 'explicit' and I have not regretted it.

In jsscope.h:
>+static inline PropertyOp
>+CastAsPropertyOp(js::Class *clasp)
>+{
>+    return JS_DATA_TO_FUNC_PTR(PropertyOp, clasp);
>+}
>+

Instead, I propose:

  protected:
    /* Used by JSEmptyScope. */
    JSScopeProperty(JSContext *cx, Class *aclasp)
      : JSScope(js_GenerateShape(cx, false)),
        id(JSVAL_EMPTY), clasp(aclasp), rawSetter(NULL),
        slot(JSSLOT_FREE(aclasp)),
        attrs(0), flags(SHARED_EMPTY), shortid(0), parent(NULL), kids(NULL)
    {}

and then in jsscopeinlines.h:

  inline
  JSEmptyScope::JSEmptyScope(JSContext *cx, js::Class *clasp)
    : JSScopeProperty(cx, clasp)
  {}

...eliminating the need for CastAsPropertyOp(js::Class *).

I think what we're really doing in jsscope.h is: there's a base class Shape
(having at least id) and two subclasses, PropertyShape and EmptyShape. Empties
are always tagged with id==JSID_EMPTY. Whether we actually want to express it
that way, I don't know, but it would get 'clasp' out of the union with
rawGetter and getterObj. You could get 'next' out of there too, by changing the
GC to use a third subclass, FreeShape (tagged with id==JSID_VOID).

(Or, rather than subclasses, there could be a three-member union in
class Shape.)

Just a thought.

> inline void
>-JSScope::removeLastProperty()
>+JSObject::removeLastProperty()
> {
>     JS_ASSERT(!inDictionaryMode());
>     JS_ASSERT_IF(lastProp->parent, !JSID_IS_VOID(lastProp->parent->id));
> 
>     lastProp = lastProp->parent;
>-    if (hash)
>-        --hash->entryCount;
> }

You can now assert that lastProp->parent is non-null, right? Then the second
assertion can drop the _IF.

In jsscopeinlines.h:

JSObject::createEmptyScope, getEmptyScope, and ensureEmptyScope can be
collapsed into a single function:

inline JSEmptyScope *
JSObject::getEmptyScope(JSContext *cx, js::Class *aclasp)
{
    if (emptyScope)
        JS_ASSERT(aclasp == emptyScope->getClass());
    else
        emptyScope = JSEmptyScope::create(cx, aclasp);
    return emptyScope;
}

> inline bool
> JSObject::canProvideEmptyScope(js::Class *aclasp)
> {
>-    /*
>-     * An empty scope cannot provide another empty scope, or wrongful two-level
>-     * prototype shape sharing ensues -- see bug 497789.
>-     */
>-    if (scope()->isSharedEmpty())
>-        return false;
>     return !emptyScope || emptyScope->clasp == aclasp;
> }

It's nice to see the end of that!

In JSObject::trace:
>-        if (!sprop || hasOwnShape()) {
>+        if (hasOwnShape()) {
>             newShape = js_RegenerateShapeForGC(cx);
>             JS_ASSERT_IF(sprop, newShape != sprop->shape);
>+            ownShape = newShape;
>         }

The ASSERT_IF doesn't need the IF part.

>     if (sprop) {
>-        JS_ASSERT(scope->hasProperty(sprop));
>+        JS_ASSERT(hasProperty(sprop));

This assertion is now vacuous.

In jstracer.cpp:
> inline LIns*
>-TraceRecorder::shape_ins(LIns* obj_ins)
>+TraceRecorder::shape_ins(LIns* obj_ins, JSObject *obj)
> {
>+    if (obj->hasOwnShape()) {
>+        return addName(lir->insLoad(LIR_ldi, obj_ins, int(offsetof(JSObject, ownShape)),
>+                                    ACCSET_OTHER),
>+                       "ownShape");
>+    }
>+
>     LIns* map_ins =
>         addName(lir->insLoad(LIR_ldp, obj_ins, int(offsetof(JSObject, map)), ACCSET_OTHER), "map");
> 
>     return addName(lir->insLoad(LIR_ldi, map_ins, offsetof(JSScope, shape), ACCSET_OTHER), "shape");
> }

A later patch will revert this, but it looks like the then-unused second
parameter remains. Kill it?
Some chatter about PropertyTables:

<brendan> does removeProperty move the table?
<brendan> no, only addPropertyCommon and putProperty do
<jorendorff> hmm -- maybe it is impossible to observe the bug
<brendan> it loses useful hashtables tho
<jorendorff> but it would have a performance impact
<jorendorff> yeah
...
<brendan> i think moving the table along whether in dictionary mode or not in addPropertyCommon is good, though
<brendan> hmm, but only so long as an overlong list gets maybeHash'ed
<brendan> which doesn't get called enough :-(
<brendan> ok, a followup patch needed to systematize this
<jorendorff> ok
<jorendorff> i'll mention
<brendan> JSC has a harsher invariant, so they can keep count of "transitions" (freeslot - JSSLOT_FREE(clasp)) basically by always hashing on any no-slot-added transition
<brendan> that ensures they can use the equiv. of table->entryCount
<brendan> and it requires some complicated pin-hash-table-to-structure bit and logic
<brendan> not too complicated but i'd prefer lazy (and frozen for non-dicts -- should assert that)
<brendan> allows stealing/thawing/refreezing
I missed where addPropertyCommon moves the table, because it doesn't call setTable. And AddPropertyHelper probably shouldn't mess with tables at all. That just leaves changeProperty and removeProperty.
(In reply to comment #93)
> Heh. It seems a little random, sometimes, what remained in JSScope and what was
> moved! In fact... in one or two cases it seems like the wrong choice:

I moved search for a reason that isn't coming to mind right now, but generally I tried to leave JSScope as a base of JSScopeProperty (now js::Shape).

> >+inline bool
> >+JSScope::isSharedEmpty() const
> >+{
> >+    return ((JSScopeProperty *)this)->flags & JSScopeProperty::SHARED_EMPTY;
> >+}
> 
> That cast reads like a signed confession ;-). If this predicate only
> applies to JSScopeProperties, why not make it a member of JSScopeProperty?

Cuz there are ~20 hits for it that I'd have to muck with, that test obj->scope()->isSharedEmpty() or similar, and I was trying to put off scope-as-type/method elimination while getting rid of the separate allocation.

> Once--in DOS days--I had a bug where my code said
>   someFunction(3);
> That was wrong. The function didn't take an int; it took an object of some
> class--it might as well have been JSScope--it was big and 3 was a totally
> meaningless argument to its constructor. But it did have a non-explicit
> constructor that took a single int argument, so C++ was implicitly converting 3
> to that type for me. This was ridiculously hard to debug. Partly because the
> crash left me in some goofy VGA graphics mode from which my only recourse was
> to reboot, sure. But from then on I used 'explicit' and I have not regretted
> it.

This was the thing I was on about in comment 90 ("constructor-style initialization -- but it's not mandatory ... and you can forget the declarator name and make a useless call, no error"), forgetting about explicit. I will use it. (But constructor-style init still looks weird! :-P)

> (Or, rather than subclasses, there could be a three-member union in
> class Shape.)
> 
> Just a thought.

Don't want sizes to vary, though.

Good suggestion on eliminating the nasty Cast(Class)AsPropertyOp, thanks.

> You can now assert that lastProp->parent is non-null, right? Then the second
> assertion can drop the _IF.

Thanks, I've been cleaning up these but missed this (I was grepping for shape->parent). More may linger, keep an eye out.

> JSObject::createEmptyScope, getEmptyScope, and ensureEmptyScope can be
> collapsed into a single function:
> 
> inline JSEmptyScope *
> JSObject::getEmptyScope(JSContext *cx, js::Class *aclasp)
> {
>     if (emptyScope)
>         JS_ASSERT(aclasp == emptyScope->getClass());
>     else
>         emptyScope = JSEmptyScope::create(cx, aclasp);
>     return emptyScope;
> }

Nice! I was conserving too much there.

I will push another mq patch for the comments not processed last time, plus of course rebasing, probably tomorrow afternoon.

/be
Partial review of patch 12 (merge-scope-and-sprop), continued:

Does anything in this stack affect the values of flat closure upvars or the
shortids of Block properties etc.? If so, bump JSXDR_BYTECODE_VERSION.

Does JSID_EMPTY have to be public? We never actually return that value from any
APIs or pass it to any callbacks, right?

>-        JS_ASSERT(obj2->scope()->hasProperty(sprop));
>+        JS_ASSERT(obj2->hasProperty(sprop));

This one is really too much: the JSObject::hasProperty method added in
fatten-object, with three arguments, is ES5 [[HasProperty]]. But the two
hasProperty methods with one argument that this patch moves from JSScope to
JSObject are not, not even for native objects! 

Rename the JSScope-ish ones? Perhaps nativeAlreadyHasProperty.

> inline JSScopeProperty *
>-JSScope::lookup(jsid id)
>+JSObject::lookup(jsid id)
> {
>     return SPROP_FETCH(search(id, false));
> }

I think all the JSObject methods that came from JSScope (I guess you can find
them by searching in jsscope.cpp and jsscopeinlines.h for "JSObject::") should
get "JS_ASSERT(isNative());" at the beginning. The ->scope() call used to
assert that. Now that assertion coverage is lost.

Better, make NativeObject a subclass of JSObject and make the downcast
explicit: obj->native()->lookup(id); </pipedream>

In jsbuiltins.cpp, AddPropertyHelper:
>     if (scope->isSharedEmpty()) {
>         scope = js_GetMutableScope(cx, obj);
>         if (!scope)
>             return false;
>     } else {
>         JS_ASSERT(!obj->hasProperty(sprop));
>     }

This assertion seems unnecessary. We already asserted that
sprop->parent == obj->lastProperty(). Are we checking for loops in the
sprop->parent chain?  But in that case hasProperty will never return...

In jsdbgapi.cpp, JS_GetPropertyDescArray:
>-    /* have no props, or object's scope has not mutated from that of proto */
>-    scope = obj->scope();
>-    if (!scope->lastProperty()) {
>+    /* have no props, or object has not mutated from that of proto */
>+    if (!obj->lastProperty()) {

The original code means "if this object has no own properties". The new
if-condition is never true.

In jsinterp.cpp, CASE(JSOP_SETNAME):
>+            if (!entry->adding()) {
>+                if ((entry->vcapTag() == 0)
>+                    ? sprop == obj->lastProperty() || obj->hasProperty(sprop)
>+                    : (obj2 = obj->getProto()) && obj2->shape() == entry->vshape())
>+                {

The new code here is so much better.

I think the hasProperty() check quoted above can be removed:

             if (!entry->adding()) {
                 if ((entry->vcapTag() == 0)
-                    ? sprop == obj->lastProperty() || obj->hasProperty(sprop)
-                    : (obj2 = obj->getProto()) && obj2->shape() == entry->vshape())
+                    || ((obj2 = obj->getProto()) && obj2->shape() == entry->vshape()))
                 {

and the assertions beefed up:

-                    JS_ASSERT(entry->directHit() ||
-                              (entry->vcapTag() == 1 && entry->kshape != entry->vshape()));
+#ifdef DEBUG
+                    if (entry->directHit()) {
+                        JS_ASSERT(obj->hasProperty(*shape));
+                    } else {
+                        JS_ASSERT(obj2->hasProperty(*shape));
+                        JS_ASSERT(entry->vcapTag() == 1);
+                        JS_ASSERT(entry->kshape != entry->vshape());
+                        JS_ASSERT(!shape->hasSlot());
+                    }
+#endif

In jspropertycache.cpp:
>-                if (sprop->parent) {
>-                    kshape = sprop->parent->shape;
>-                } else {
>-                    /*
>-                     * If obj had its own empty scope before, with a unique
>-                     * shape, that is lost. Here we only attempt to find a
>-                     * matching empty scope. In unusual cases involving
>-                     * __proto__ assignment we may not find one.
>-                     */
>-                    JSObject *proto = obj->getProto();
>-                    if (!proto || !proto->isNative())
>-                        return JS_NO_PROP_CACHE_FILL;
>-                    if (!proto->emptyScope || proto->emptyScope->clasp != obj->getClass())
>-                        return JS_NO_PROP_CACHE_FILL;
>-                    kshape = proto->emptyScope->shape;
>-                }
>+                kshape = sprop->parent->shape;

*Great* to see that nonsense gone.
Oh btw, here's what comment 94 and 95 are talking about. (I thought I had posted this stuff in comment 93. Oops.)

In jsscope.cpp, JSObject::putProperty:
>+            /* Move table from oldLastProp to the new lastProp, aka sprop. */
>+            JS_ASSERT(oldLastProp->table == table);
>+            oldLastProp->table = NULL;
>+            sprop->table = table;

This happens in putProperty and in addPropertyCommon, but I think it should
also happen in changeProperty and removeProperty.

JSObject::changeProperty says:
>+    if (inDictionaryMode()) {
>         removeDictionaryProperty(sprop);
>+        newsprop = newDictionaryProperty(cx, child, &lastProp);
>         if (newsprop) {
>+            if (lastProp->table) {
>+                JSScopeProperty **spp = search(sprop->id, false);
>                 SPROP_STORE_PRESERVING_COLLISION(spp, newsprop);
>             }
>             updateShape(cx);
>         }
>+    } else [...]

It updates the table but does not move it. sprop then becomes garbage, so there
is no observably buggy behavior; but we have lost the hash table.
Review of patch 12 (merge-scope-and-sprop), concluded:

In jsscope.cpp, JSScope::finishRuntimeState:
>+    rt->emptyArgumentsScope = NULL;;
>+    rt->emptyBlockScope = NULL;;
>+    rt->emptyCallScope = NULL;;
>+    rt->emptyDeclEnvScope = NULL;;
>+    rt->emptyEnumeratorScope = NULL;;
>+    rt->emptyWithScope = NULL;;

Doubled ;s.

In JSObject::addPropertyCommon:
>    /* Check whether we need to grow, if the load factor is >= .75. */
>    PropertyTable *table = NULL;
>    if (!inDictionaryMode()) {
>        if (lastProp->entryCount() > MAX_PROPERTY_TREE_HEIGHT) {
>            if (!toDictionaryMode(cx))
>                return NULL;
>            spp = search(id, true);
>            table = lastProp->table;
>        }
>    } else if ((table = lastProp->table) != NULL) {
>        uint32 size = table->capacity();
>        if (table->entryCount + table->removedCount >= size - (size >> 2)) {

The comment is pretty far from the code it's talking about.

r=me with the changes requested. Comment 98 in particular.
(In reply to comment #84)
> In js_SetReservedSlot and again in js_DumpObject, comments and code about
> freeslot being an underestimate were deleted, but in js_TraceObject they were
> retained. What changed?

The js_TraceObject comment and code say:

    if (!scope->isSharedEmpty() && IS_GC_MARKING_TRACER(trc)) {
        /*
         * Check whether we should shrink the object's slots. Skip this check
         * if the scope is shared, since for Block objects and flat closures
         * that share their scope, freeslot can be an underestimate.
         */
        size_t slots = obj->freeslot;
        if (obj->numSlots() != slots)
            obj->shrinkSlots(cx, slots);
    }

This bug's patch queue in full fixes the flat closure case, but block objects still need to be spared this test. They can't escape, so they are not going to be extended by js_AddProperty/AddPropertyHelper (ditto Call objects, but those both can be extended by ad-hoc properties created by eval, and have a proper compiler-created map/lastProp).

This suggests strongly that I go the full distance now, as I've gone for Call, flat closure Function, and Arguments objects, and make cloned Blocks share their prototype's shape map. Then I believe I could get rid of the !isSharedEmpty() test shown above.

JSObject::ensureClassReservedSlotsForEmptyObject is still needed, for cases such as Date objects which (now, thanks to bhackett's perf work) have a large number of class-reserved slots. I could fold it into JSObject::allocSlot() but I opted to keep it separate, called only if isSharedEmpty(), with that test inlined. This is in all the places that used to call js_GetMutableScope.

Comments on any of this? I'll work on removing the special case for Blocks.

/be
The js_TraceObject freeslot-may-under-estimate comment and code (testing isSharedEmpty()) is just wrong. Ripping it out.

I will avoid doing anything to Blocks in this over-deep patch queue, however: they would need some care to avoid allocation of slots to compile-time Block objects, while avoiding JSPROP_SHARED in the shared shapes in order actually to allocate and shape-map the slots of cloned Blocks.

/be
(In reply to comment #100)
> JSObject::ensureClassReservedSlotsForEmptyObject is still needed, for cases
> such as Date objects which (now, thanks to bhackett's perf work) have a large
> number of class-reserved slots. I could fold it into JSObject::allocSlot() but
> I opted to keep it separate, called only if isSharedEmpty(), with that test
> inlined. This is in all the places that used to call js_GetMutableScope.

The idea is to gain better branch prediction by having more sites that do or do not see isSharedEmpty() predictably -- as opposed to putting the test down in allocSlots, with one branch site.

This may be silly, though. I will try to measure whether it matters. More than any slight win, at the cost of some code bloat and maintenance, if we do indeed reach the flexible fslots (at the end of JSObject and its subtypes) promised land, we won't mind preallocating all the class-reserved slots -- even the 9 (!) for Date instances.

Right now it would noticeably hurt, I bet, to allocate a separate dslots for all Date instances, when only some (those for which, e.g., getYear or like methods have been called) need the cache space. But with a single consolidated GC-thing allocation, probably the eager fslots allocation wouldn't hurt -- and we could even say all class-reserved slots must be fslots.

/be
(In reply to comment #92)
> Created attachment 467946 [details] [diff] [review]
> make dumpObject print JSObject::flags
> 
> Threw this together while poking at the next patch. (So far, I don't think I've
> found any bugs in the entire stack.)

Thanks, good stuff. I'll make good use of hasPropertyTable.

/be
There's another freeslot issue in js_TraceObject:

    uint32 nslots = obj->numSlots();
    if (!scope->isSharedEmpty() && obj->freeslot < nslots)
        nslots = obj->freeslot;

The e4x/GC/regress-292455.js test fails is I remove the !scope->isSharedEmpty() test (or equivalent, modulo changes for review comments about moving it over to JSObject and renaming it empty()).

Turns out js_QNameClass has no 3 reserved slots but nothing sets obj->freeslot for an unmutated QName instance -- only if one adds an ad-hoc property does the JSObject::ensureClassReservedSlots machinery kick in.

But this is not atypical. Unfortunately, the original design of scope sharing among unmutated (by ad-hoc properties) kids and their prototype object meant that scope->freeslot was not usable by the kids. OTOH, marking the proto's unallocated slots was a waste too (but safe, since the slots are all set to undefined, a waste of cycles and an allocation path perf hit).

So kids sharing a proto-scope would have all their nslots scanned. The proto's freeslot or nslots, which ever was smaller, would bound its slots-scan.

This means we have more than just E4X objects counting on both sides of the coin. It's like Postel's Law and the corollary I'm mis-credited with. Date objects have 9 reserved slots, requiring dslots allocation, but neither tm tip nor my patch give such a Date instance its own scope or shape and a freeslot covering the class-reserved slots. This would be wasteful, since the instance lacks ad-hoc properties.

I'm going to keep this js_TraceObject !empty condition and comment it. Followup work in bug 584917 and possibly others should address the complexity here by either requiring a slots-covering shape (as I've done for Call objects here) or else putting the reserved state elsewhere, ideally in well-typed data members of implementation (not just interface) subclasses of JSObject.

/be
(In reply to comment #97)
> Partial review of patch 12 (merge-scope-and-sprop), continued:
> 
> Does anything in this stack affect the values of flat closure upvars or the
> shortids of Block properties etc.?

No, or you'd see emitter and decompiler changes at the least.

> Does JSID_EMPTY have to be public? We never actually return that value from any
> APIs or pass it to any callbacks, right?

Talked to Luke about it a while ago, this seemed ok, wasn't obvious how to hide it. Suggestions?

> >-        JS_ASSERT(obj2->scope()->hasProperty(sprop));
> >+        JS_ASSERT(obj2->hasProperty(sprop));
 . . .
> Rename the JSScope-ish ones? Perhaps nativeAlreadyHasProperty.

I will work on naming. Moving stuff around to refactor without lingering on names, but this isn't the only one that is confusing. Good point.

> I think all the JSObject methods that came from JSScope (I guess you can find
> them by searching in jsscope.cpp and jsscopeinlines.h for "JSObject::") should
> get "JS_ASSERT(isNative());" at the beginning. The ->scope() call used to
> assert that. Now that assertion coverage is lost.

You egged me on to eliminating JSScope, so scope() is gone (too tired to attach now; tomorrow). JSObject::lastProperty() asserts isNative().

> Better, make NativeObject a subclass of JSObject and make the downcast
> explicit: obj->native()->lookup(id); </pipedream>

I'm not sure we want to go there yet but we're all talking about subtypes for interface specialization, if not for implementation (data members).

I'm game, in a later patch and in general. For this case the alternative of isNative() <=> !isProxy() seems even better, and suggests if-based dispatch.

> In jsbuiltins.cpp, AddPropertyHelper:
> >     if (scope->isSharedEmpty()) {
> >         scope = js_GetMutableScope(cx, obj);
> >         if (!scope)
> >             return false;
> >     } else {
> >         JS_ASSERT(!obj->hasProperty(sprop));
> >     }
> 
> This assertion seems unnecessary. We already asserted that
> sprop->parent == obj->lastProperty(). Are we checking for loops in the
> sprop->parent chain?  But in that case hasProperty will never return...

No, this was more about the object's property table, if it exists. But it is now a part of the shape, so vacuous. Yanking.

> In jsdbgapi.cpp, JS_GetPropertyDescArray:
> >-    /* have no props, or object's scope has not mutated from that of proto */
> >-    scope = obj->scope();
> >-    if (!scope->lastProperty()) {
> >+    /* have no props, or object has not mutated from that of proto */
> >+    if (!obj->lastProperty()) {
> 
> The original code means "if this object has no own properties". The new
> if-condition is never true.

Good catch. As noted previously I worry there are more like this one. They are hard to see. Let me know -- thanks.

/be
Assignee: jorendorff → brendan
Review for patch 13 (rm-empty-shapes):

In jspropertytree.h:
> struct ShapeHasher {
>     typedef js::Shape *Key;
>     typedef const js::Shape *Lookup;
> 
>     static HashNumber hash(const Lookup l);
>     static bool match(const Key k, const Lookup l);
> };

Note that `const Key` does not mean `const js::Shape *` ("pointer to const
Shape") but rather `js::Shape * const` ("const pointer to (mutable)
Shape"). Since we don't use types like that anywhere else, it's weird to see
them here.

I don't think the typedefs aren't making things any clearer here, so maybe
just:
    static HashNumber hash(const js::Shape *l);
    static bool match(const js::Shape *k, const js::Shape *l);

In jspropertytree.h, class KidsPointer:
>+    bool isShape() const { return (w & TAG) == SHAPE; }

I'm not crazy about this returning true for null kids, for the same reason that
NullValue()->isObject() being false seems only sane. For speed, you could add
an isShapeOrNull version with this fast implementation.

(But ideally you shouldn't have to; all the code is like

    if (kids.isNull())
        ...
    else if (kids.isShape())
        ...

and the compiler could in principle optimize away an extra null-check in
isShape.)

>+    JSScopeProperty *toSprop() const { JS_ASSERT(isSprop());
>+        return reinterpret_cast<JSScopeProperty *>(w & ~jsuword(TAG));
>+    }
>+    void setSprop(JSScopeProperty *sprop) {
>+        JS_ASSERT(sprop);
>+        JS_ASSERT((reinterpret_cast<jsuword>(sprop) & TAG) == 0);
>+        w = reinterpret_cast<jsuword>(sprop);
>+    }

Super-micro-nit: Either remove the useless bitwise AND in toSprop (now
toShape); or keep it and add the corresponding useless `| SHAPE` in setShape.

In PropertyTree::insert:
Whew -- I have a hard time believing the old code or the new code is
threadsafe, especially once a hash table is involved. I'll be glad when we can
remove the nice comments about what supposedly happens when we
race... *whistling past graveyard*

I wonder if supporting multiple chunks buys us anything over just making the
chunk size our cutoff point and switching to a hash table then.

In jspropertytree.cpp, HashChunks:
>+            Hash::AddPtr addPtr = hash->lookupForAdd(sprop);
>+            if (!addPtr) {
>+#ifdef DEBUG
>+                bool ok =
>+#endif
>+                    hash->add(addPtr, sprop);
>+                JS_ASSERT(ok);

Is this kind of JS_ASSERT(ok) OK now?

In jspropertytree.cpp, PropertyTree::getChild:
>+        } else if (kidp->isChunk()) {
>+            Chunk *chunk = kidp->toChunk();
>+
>+            uintN n = 0;
>+            do {
>+                for (uintN i = 0; i < MAX_KIDS_PER_CHUNK; i++) {
>+                    sprop = chunk->kids[i];
>+                    if (!sprop) {
>+                        n += i;
>+                        if (n >= CHUNK_HASH_THRESHOLD) {
>+                            if (!kidp->isHash()) {

Is this second 'if' necessary? (It can only miss if another racing thread came
through this code and hashed this node; if that's what this is about, I think
it's a weird enough case to deserve a one-line comment.)

In OrphanNodeKids:
>+    JS_ASSERT(!sprop->parent || sprop->parent->kids.isNull() ||
>+              !sprop->parent->kids.isSprop());

If you change isSprop() to exclude isNull(), the isNull() part here can be
removed.

In PropertyTree::sweepShapes:
>     /*
>-     * First, remove unmarked nodes from JS_PROPERTY_TREE(cx).hash. This table
[...]
>-     * Second, if any empty scopes have been reshaped, rehash the root ply of
[...]
>      * Third, sweep the heap clean of all unmarked nodes. Here we will find

The first and second steps were removed, so now the first comment starts with
"Third".


JSEmptyScope::JSEmptyScope says:
>+    kids.setNull();

And PropertyTree::getChild says:
>     new (shape) Shape(child.id, child.rawGetter, child.rawSetter, child.slot, child.attrs,
>                       child.flags, child.shortid);
>     shape->parent = NULL;
>     shape->kids.setNull();

kids is a member of JSScope; why not initialize it in JSScope's constructor?

r=me with these nits picked.
Thanks for the discussion of lazy freeslot initialization in comment 100 ff. The conclusion in comment 104:
> I'm going to keep this js_TraceObject !empty condition and comment it.
sounds good to me.

(In reply to comment #105)
> > Does JSID_EMPTY have to be public? We never actually return that value from any
> > APIs or pass it to any callbacks, right?
> 
> Talked to Luke about it a while ago, this seemed ok, wasn't obvious how to hide
> it. Suggestions?

It can't just go in a private header? Perhaps jsscope.h?

> I'm game, in a later patch and in general. For this case the alternative of
> isNative() <=> !isProxy() seems even better, and suggests if-based dispatch.

if-based dispatch for sure.

> Good catch. As noted previously I worry there are more like this one. They are
> hard to see. Let me know -- thanks.

I didn't see any more, and I was looking for them.
Review for patch 14 (rm-js_GetMutableScope):

In jsinterp.cpp, case JSOP_SETMETHOD:
>-                     * We check that cx owns obj here and will continue to own
>+                     * We check that cx own obj here and will continue to own

"owns" was right.

In jsobj.cpp, js_NonEmptyObject:
> {
>     JS_ASSERT(!(js_ObjectClass.flags & JSCLASS_HAS_PRIVATE));
>     JSObject *obj = js_NewObjectWithClassProto(cx, &js_ObjectClass, proto, UndefinedValue());
>     if (!obj)
>         return NULL;
>     JS_LOCK_OBJ(cx, obj);
>-    JSScope *scope = js_GetMutableScope(cx, obj);
>-    if (!scope) {
>+    if (!obj->ensureClassReservedSlotsForEmptyObject(cx)) {
>         JS_UNLOCK_OBJ(cx, obj);
>         return NULL;
>     }
> 
>     /*
>      * See comments in the JSOP_NEWINIT case of jsinterp.cpp why we cannot
>      * assume that cx owns the scope and skip the unlock call.
>      */

The locking and the call to ensureClassReservedSlotsForEmptyObject can be
removed. (JSOP_NEWINIT doesn't do that stuff anymore, and the cited comments
are gone.)

In JSObject::ensureClassReserveSlotsForEmptyObject:
>+     * Subtle rule: [...] All of this will go away soon (FIXME: bug
>+     * 558451).

New follow-up bug number here?

r=me with those changes.
Review for patch 15 (rename-sprop):

In jsscope.h, what's now Shape::insertIntoDictionary:
>      * Don't assert inDictionaryMode() here because we may be called from
>      * JSObject::toDictionaryMode via JSObject::newDictionaryProperty.

That's now newDictionaryShape.

In jsscopeinlines.h:
>+js::Shape::Shape(jsid id, js::PropertyOp getter, js::PropertyOp setter,
>                                  uint32 slot, uintN attrs, uintN flags, intN shortid)

Indentation.

> JSEmptyScope::JSEmptyScope(JSContext *cx, js::Class *aclasp)
>-  : Shape(JSID_EMPTY, js::CastAsPropertyOp(aclasp), NULL, JSSLOT_FREE(aclasp),
>+  : js::Shape(JSID_EMPTY, js::CastAsPropertyOp(aclasp), NULL, JSSLOT_FREE(aclasp),
>                     0, SHARED_EMPTY, 0)

Also here.

In jsscope.cpp, JSObject::clear:
>-    JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals);

Just trying to see if I was paying attention? :)

I don't see why it makes sense to remove this and retain the one in
JSObject::removeProperty. The comment on JSRuntime::propertyRemovals needs
to be updated, in any case: it still claims it "is incremented for every
JSObject::clear".

r=me with those nits picked.
Comment on attachment 467946 [details] [diff] [review]
make dumpObject print JSObject::flags

Integrated, thanks!

/be
Attachment #467946 - Flags: review?(brendan) → review+
I'm skipping patch 16 for now. I'll come back to it.

Review for patch 17 (args-and-call-cleanup):

In jsfun.cpp, js_Arguments:
>         /*
>          * Strict mode callers must copy arguments into the created arguments
>-         * object.
>+         * object. XYZZYbe what about tracing?

This code seems fine. What's wrong?

In jsobj.h, JSObject::ensureInstanceReservedSlots:
>+     * This method may be called only for native objects freshly created using
>+     * NewObject or one of its variant where the new object will both (a) never
>+     * escape to script and (b) never be extended with ad-hoc properties that
>+     * would try to allocate higher slots.

I don't think (b) is true of Call objects in functions that call eval.

r=me with those comments addressed.
(In reply to comment #106)
> I don't think the typedefs aren't making things any clearer here, so maybe
> just:
>     static HashNumber hash(const js::Shape *l);
>     static bool match(const js::Shape *k, const js::Shape *l);

As mentioned on IRC, this resulted in template mismatch hell, which is why I did what I did (with obi-luke advising). But I found that stripping the consts from the static bool match(Key k, Lookup l); worked too, and went with that. Some StripConst magic at work?

> (But ideally you shouldn't have to; all the code is like
> 
>     if (kids.isNull())
>         ...
>     else if (kids.isShape())
>         ...
> 
> and the compiler could in principle optimize away an extra null-check in
> isShape.)

Added both but don't use OrNull flavor, compiler will avoid redundant 0 w test I am sure (HAH!).

> Super-micro-nit: Either remove the useless bitwise AND in toSprop (now
> toShape); or keep it and add the corresponding useless `| SHAPE` in setShape.

Did the cleaner thing (the latter).

> In PropertyTree::insert:
> Whew -- I have a hard time believing the old code or the new code is
> threadsafe, especially once a hash table is involved. I'll be glad when we can
> remove the nice comments about what supposedly happens when we
> race... *whistling past graveyard*

The old code allowed races to insert, so duplicate kids. But it always had chunks. No transition to hash in the old code.

New code as noted in FIXME comments in the patch q is racy as hell. Come get some, to quote Ash Williams talking smack to a Deadite ;-).

> I wonder if supporting multiple chunks buys us anything over just making the
> chunk size our cutoff point and switching to a hash table then.

It's a good question. I didn't want to fiddle in this queue but I noticed that chunks are always more space-efficient given any hash that isn't overloaded. So we need to trade space away for time. A followup bug, I promise.

> In jspropertytree.cpp, HashChunks:
> >+            Hash::AddPtr addPtr = hash->lookupForAdd(sprop);
> >+            if (!addPtr) {
> >+#ifdef DEBUG
> >+                bool ok =
> >+#endif
> >+                    hash->add(addPtr, sprop);
> >+                JS_ASSERT(ok);
> 
> Is this kind of JS_ASSERT(ok) OK now?

This is based on hash->init(n) "right-sizing" the table including overhead to keep it from being overloaded, but yeah: it could be removed. Done.

> In jspropertytree.cpp, PropertyTree::getChild:
> >+        } else if (kidp->isChunk()) {
> >+            Chunk *chunk = kidp->toChunk();
> >+
> >+            uintN n = 0;
> >+            do {
> >+                for (uintN i = 0; i < MAX_KIDS_PER_CHUNK; i++) {
> >+                    sprop = chunk->kids[i];
> >+                    if (!sprop) {
> >+                        n += i;
> >+                        if (n >= CHUNK_HASH_THRESHOLD) {
> >+                            if (!kidp->isHash()) {
> 
> Is this second 'if' necessary? (It can only miss if another racing thread came
> through this code and hashed this node; if that's what this is about, I think
> it's a weird enough case to deserve a one-line comment.)

I'm itching to simplify this code to be single-threaded and move it over to jsshape.cpp/.h, but one more comment won't break me.

> In OrphanNodeKids:
> >+    JS_ASSERT(!sprop->parent || sprop->parent->kids.isNull() ||
> >+              !sprop->parent->kids.isSprop());
> 
> If you change isSprop() to exclude isNull(), the isNull() part here can be
> removed.

And then I can use JS_ASSERT_IF(sprop->parent, ...) -- nice!

> JSEmptyScope::JSEmptyScope says:
> >+    kids.setNull();
> 
> And PropertyTree::getChild says:
> >     new (shape) Shape(child.id, child.rawGetter, child.rawSetter, child.slot, child.attrs,
> >                       child.flags, child.shortid);
> >     shape->parent = NULL;
> >     shape->kids.setNull();
> 
> kids is a member of JSScope; why not initialize it in JSScope's constructor?

Can't do it if it's in a union, which it is. Was C++0x or 1x fixing this botch?

I did common to exactly one kids.setNull(), in Shape::Shape. Thanks,

/be
(In reply to comment #107)
> > > Does JSID_EMPTY have to be public? We never actually return that value from any
> > > APIs or pass it to any callbacks, right?
> > 
> > Talked to Luke about it a while ago, this seemed ok, wasn't obvious how to hide
> > it. Suggestions?
> 
> It can't just go in a private header? Perhaps jsscope.h?

I would have to copy around some JS_USE_JSVAL_JSID_STRUCT_TYPES ifdefs. I was happy to leave this alone if Luke was happy with it. Maybe he isn't?

> > Good catch. As noted previously I worry there are more like this one. They are
> > hard to see. Let me know -- thanks.
> 
> I didn't see any more, and I was looking for them.

Yeah, I did `grep -w if *.cpp *.h | grep lastProp` and didn't see any. Thanks again.

Re: comment 108:

Nice js_NonEmptyObject simplification. Thanks!

I rewrote that ungrammatical JSObject::ensureClassReservedSlotsForEmptyObject comment and lost the FIXME. I'll file followups based on review of FIXMEs and comments here.

Re: comment 109:

Indentation fixed and I also opened namespace js to avoid noxioius js:: prefixes.

JS_ATOMIC_INCREMENT(&cx->runtime->propertyRemovals); -- whoops! You are awake!

(In reply to comment #111)
> In jsfun.cpp, js_Arguments:
> >         /*
> >          * Strict mode callers must copy arguments into the created arguments
> >-         * object.
> >+         * object. XYZZYbe what about tracing?
> 
> This code seems fine. What's wrong?

Nothing, I cited TraceRecorder::newArguments to help the next person as blind as me find the way.

> In jsobj.h, JSObject::ensureInstanceReservedSlots:
> >+     * This method may be called only for native objects freshly created using
> >+     * NewObject or one of its variant where the new object will both (a) never
> >+     * escape to script and (b) never be extended with ad-hoc properties that
> >+     * would try to allocate higher slots.
> 
> I don't think (b) is true of Call objects in functions that call eval.

The previous patch in the queue, 19 A bug558451-no-call-object-dslots, covered the slots allocated by calling ensureInstanceReservedSlots via the compiler-created shape path from the callee function. But yeah, the comment is wrong. Fixed, thanks.

/be
Attachment #467656 - Attachment is obsolete: true
Attachment #467946 - Attachment is obsolete: true
Attachment #468569 - Flags: review?(jorendorff)
Attachment #467656 - Flags: review?(jorendorff)
Attachment #468569 - Attachment is patch: false
Attachment #468569 - Attachment mime type: text/plain → application/x-tar
Review for patch 18 (use-shapes-for-fun-u.i.names):

In jsapi.cpp, JS_CloneFunctionObject:
>+        for (Shape::Range r = Shape::Range(fun->lastUpvar()); i-- != 0; r.popFront()) {

I'd prefer:
          for (Shape::Range r(fun->lastUpvar()); i-- != 0; r.popFront()) {
which is how you write it in JSFunction::findDuplicateFormal.

In jsfun.cpp, JSFunction::addLocal:
>+     * The destructuring formal parameter parser adds a null atom, which we
>+     * encode as a VOID id. [...]

We don't actually encode that as a VOID id, do we? It looks like we use an int.

>+    if (!shape->inDictionary())
>+        u.i.names = shape;

Is this necessary? Isn't listp = &u.i.names in this case, so that getChild()
already set it for us?

In jsscope.h:
>+JS_ALWAYS_INLINE js::Shape **
>+js::Shape::search(js::Shape *&start, jsid id, bool adding)

You mentioned getting rid of "*&" in this file and I appreciate it.
I like js::Shape ** much better for the parameter here.

r=me with that.
Review for patch 19 (no-call-object-dslots): r=me.

Righteous patch!
Review for patch 20 (no-args-element-dslots):

In jsfun.cpp, args_or_call_trace:
>+        ArgumentsData *data = obj->getArgsData();
>+        if (data->callee.isObject())
>+            MarkObject(trc, &data->callee.toObject(), js_callee_str);
>+        MarkValueRange(trc, obj->getArgsInitialLength(), data->slots, js_arguments_str);

This whole function is #if JS_HAS_GENERATORS. It should be always-on now, right?

In jsobjinlines.h:
>+inline void
>+JSObject::staticAssertNoArrayFixedElementSlots()
>+{
>+    JS_STATIC_ASSERT(ARGS_FIXED_ELEMENT_SLOTS == 0);
>+}

It seems this constant is only used for this static assertion. Can we just
delete it?

r=me with that.
Review for patch 21 (no-flat-closure-upvar-dslots):

In jsfun.cpp, CallPropertyOp:
>-        JS_ASSERT(!setter);

You refactored this a little to support assigning to flat closure slots, which
presumably only the debugger could do. Can we actually hit this code now?

In jsobj.cpp:
>-    JS_ASSERT_IF(isNative() && !isCall(), scope()->isSharedEmpty());
>+    JS_ASSERT_IF(isNative() && isBlock(), scope()->isSharedEmpty());

"isNative() &&" can be removed.
Er, r=me with the nit picked.
Review for patch 22 (unify-obj-own-shape):

In jsobjinlines.h, JSObject::init:
>+     * we set map to null in DEBUG builds, and set objShape to a valid we then
>+     * assert obj->shape() never returns.

"value", not "valid".

In jsscopeinlines.h:
> inline void
> JSObject::trace(JSTracer *trc)
> {
>     JSContext *cx = trc->context;
>-    js::Shape *shape = const_cast<js::Shape *>(lastProp);
>+    js::Shape *shape = lastProp;

I am probably missing something obvious, but I don't see why it's safe to
assume this is native here (with or without this patch).

r=me with those addressed.
Status: 22 patches now have review.  4 to go. I still need to finish reviewing patch 16. That will take an hour or so. And there are three new patches, addressing earlier review comments, that now need review. Those will go quickly.
Review for patch 16 (abstract-and-constipate-shape):

In jsopcode.cpp, GetLocal:
>     for (jsatomid j = 0, n = script->objects()->length; ; j++) {
>         if (j == n)
>             return GetStr(ss, i);
> 
>         JSObject *obj = script->getObject(j);
>         if (obj->isBlock()) {
>             [...]
>         }
>     }
> 
>     JS_NOT_REACHED("QuoteString");
>     return "";

It seems like what you're saying here is:

    for (jsatomid j = 0, n = script->objects()->length; j != n; j++) {
        JSObject *obj = script->getObject(j);
        if (obj->isBlock()) {
            [...]
        }
    }
    return GetStr(ss, i);

Still in GetLocal:
>             if (jsuint(i - depth) < jsuint(count)) {
>                 i -= depth;
>                 for (Shape::Range r = obj->lastProperty()->all(); !r.empty(); r.popFront()) {
>                     const Shape &shape = r.front();
> 
>                     if (shape.shortid == i) {
>                         [...]
>                         return rval;
>                     }
>                 }

It seems like you want a JS_NOT_REACHED (and then maybe a break statement,
belt-and-braces) immediately after this loop, since `i` has been modified.

In jsscope.h, struct JSScope:
>+    void setTable(js::PropertyTable *t) const {
>+        table = t;
>+    }
>+
>+  protected:
>+    mutable js::PropertyTable *table;

This method probably shouldn't be const, and then the field doesn't have to be
mutable.

In JSScope::Range:
>+        Range(const Shape *s, const Shape *e = NULL) : cursor(s), end(e) { }

The second parameter is unused, so the `end` field can be removed.

> /*
>  * Note that shape must not be null, as emptying a scope requires extra work
>  * done only by methods in jsscope.cpp.
>  */
> inline void
>-JSObject::setLastProperty(js::Shape *shape)
>+JSObject::setLastProperty(const js::Shape *shape)
> {
[...]
>-    lastProp = shape;
>+    lastProp = const_cast<js::Shape *>(shape);
> }

Woof. The way all this constness played out is not great. The number of
const_casts may be small, but their scope, particularly here, is broad enough
to make me very skeptical of the safety property this is trying to enforce. I
don't think const was the right tool this time. But we'll live!

r=me!
Review for patch 23 (review-fixes-c81): r=me.

"this scope" still appears in several comments in jsobj.h. I think all of them
mean "this object".

Comment 73 calls for a
    JS_ASSERT(!entries);
in jsscope.cpp, PropertyTable::init.

A comment in js_SetPropertyHelper says:
>     * If JS_THREADSAFE and shape is non-null, then pobj is locked, and shape
>     * is held: [...]

"shape is held" sounds like now-osbolete refcounting jargon. (?)

(In reply to comment #122)
> Woof. The way all this constness played out is not great. The number of
> const_casts may be small, but their scope, particularly here, is broad enough
> to make me very skeptical of the safety property this is trying to enforce. I
> don't think const was the right tool this time. But we'll live!

Definitely a work in progress. I had a bug making fun->u.i.names use shapes that resulted in mutation of shared shapes. Painful enough that I constipated, even though we all know how that ends.

Shape::setTable I left const with table mutable, to avoid undoing some of this const-ipation. Shape's conceptual const-ness is not violated by table, which is an optimization (unobservable except by timing, implementation detail). But yeah, const (sigh).

/be

/be
Partial review for patch 24 (review-fixes-c99):

Comment 89 said (regadring 24-bit JSObject::freeslot):
> I'm almost certainly missing an overflow check on this. Will get to it shortly.

Did you get to it? Grepping for freeslot didn't find an overflow check, for
me. It looks like you might need it in more than one place.

In jsscope.h:
>    class Range {
>      protected:
>        friend class Shape;

This should be "friend struct Shape;" to avoid an MSVC warning.

In jsdbgapi.cpp, DropWatchPointAndUnlock
    >+     * Passing null for the obj parameter tells GetWatchedSetter to find any

There is no obj parameter to GetWatchedSetter.

In jspropertycache.cpp, PropertyCache::fill:
>-     * we know thanks to the scope->hasProperty test above, combined with the
>+     * we know thanks to the pobj->hasProperty test above, combined with the

pob->nativeContains, not hasProperty.

In jsscope.h, struct js::Shape:
>          * Set during a shape-regenerating GC if the shape has already been
>+         * regenerated. Unlike js::Shape::SHAPE_REGEN, this does not toggle
>+         * with each GC. js::PropertyTree::sweepShapes clears it.
>          */
>         SHAPE_REGEN     = 0x04,

That *is* js::Shape::SHAPE_REGEN! The amazing toggling hack is gone.
Review for patch 25 (review-fixes-111): r=me.

In jsobj,cpp:
> JSObject* FASTCALL
> js_NonEmptyObject(JSContext* cx, JSObject* proto)
> {
>     JS_ASSERT(!(js_ObjectClass.flags & JSCLASS_HAS_PRIVATE));
>     JSObject *obj = js_NewObjectWithClassProto(cx, &js_ObjectClass, proto, UndefinedValue());
>     if (!obj)
>         return NULL;
>+    if (!obj->ensureClassReservedSlotsForEmptyObject(cx))
>         return NULL;
>     return obj;
> }

If you want:
    return obj && obj->ensureBlahBlah(cx) ? obj : NULL;

In jspropertytree.cpp, HashChunks:
>             if (!addPtr) {
>-#ifdef DEBUG
>-                bool ok =
>-#endif
>-                    hash->add(addPtr, shape);
>-                JS_ASSERT(ok);
>+                hash->add(addPtr, shape);

A one-liner would be polite:
  /* OOM can be quietly ignored here. It's OK because XYZ. */
just to avoid wasting other conscientious code-readers' cycles when they notice
that the return isn't checked.

(Incidentally you can use JS_ALWAYS_TRUE(hash->add(blah)) in place of the idiom
being removed here...)
I have to go now. With the first note in comment 125 addressed, this stack has my blessing!
(In reply to comment #125)
> Partial review for patch 24 (review-fixes-c99):
> 
> Comment 89 said (regadring 24-bit JSObject::freeslot):
> > I'm almost certainly missing an overflow check on this. Will get to it shortly.
> 
> Did you get to it? Grepping for freeslot didn't find an overflow check, for
> me. It looks like you might need it in more than one place.

Only one place, but to my horror I see no defense against overflowing nslots computation under JSObject::growSlots. Did we lose that?

I restored an overflow check, very safe now since the nslots/freeslot limit is JS_BIT(24), and added assertions after each freeslot++ that freeslot != 0.

> In jsdbgapi.cpp, DropWatchPointAndUnlock
>     >+     * Passing null for the obj parameter tells GetWatchedSetter to find
> any
> 
> There is no obj parameter to GetWatchedSetter.

Thanks. I was commenting around left-over issues now that there's only one call to GetWatchedSetter, so I inlined that static helper and simplified. 

> In jsscope.h, struct js::Shape:
> >          * Set during a shape-regenerating GC if the shape has already been
> >+         * regenerated. Unlike js::Shape::SHAPE_REGEN, this does not toggle
> >+         * with each GC. js::PropertyTree::sweepShapes clears it.
> >          */
> >         SHAPE_REGEN     = 0x04,
> 
> That *is* js::Shape::SHAPE_REGEN! The amazing toggling hack is gone.

Whew! Thanks for noticing. I focused on code at the expense of comments there.

Rebasing, final testing. I will now attach tarball with bug558451-review-fixes-c127 pushed, in case you want a look.

/be
Attachment #468569 - Attachment is obsolete: true
Attachment #468569 - Flags: review?(jorendorff)
Attachment #468875 - Attachment is obsolete: true
tryserver'ed again, this is close to landing. I may do so first thing tomorrow.

/be
Also override resolve flags of 0 from fun_enumerate, for backward compatibility.

Jorendorff, hope you can take a look. Several patches changed other than due to rebasing. I will tryserver and (if green enough) land tonight or tomorrow a.m.

/be
Attachment #468940 - Attachment is obsolete: true
Attachment #469316 - Flags: review?(jorendorff)
The case of a bound function with pre-args being extended with an ad-hoc property and not being mis-cached based on a function with no pre-args and the same ad-hoc property is not covered. Too tired to do it now, Waldo or I will get to it.

js_TraceObject was brutally trimming dslots of empty Date objects, breaking the browser-chrome mochitests. Same !obj->nativeEmpty() test needed here as just a bit later to decide how many slots to mark. Duh!

/be
Attachment #469316 - Attachment is obsolete: true
Attachment #469391 - Flags: review?(jorendorff)
Attachment #469316 - Flags: review?(jorendorff)
one remaining tryserver leak failure. The problem is in the IndexDB tests.
Attachment #469391 - Attachment is obsolete: true
Attachment #469391 - Flags: review?(jorendorff)
Attached patch tryserver-fixes patch (obsolete) — Splinter Review
Soliciting review here rather than on the tgz. Feel free to check that out but I kept the changes in this patch to avoid self-conflicting.

/be
Attachment #470138 - Flags: review?(jorendorff)
Attachment #470136 - Attachment is obsolete: true
Attachment #470138 - Attachment is obsolete: true
Attachment #470304 - Flags: review?(jorendorff)
Attachment #470138 - Flags: review?(jorendorff)
I landed:

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

See also bug 477999, whose patch landed earlier today, requiring rebasing of this bug's patch, then had to be backed out but after I'd landed.

/be
Whiteboard: fixed-in-tracemonkey
With GCC 4.4 on Linux I cannot link js shell:

/home/igor/m/tm/js/src/jsobj.cpp:3587: undefined reference to `JSFunction::isBound() const'

To fix this locally I removed inline from isBound definition in jsfun.cpp.
I also get couple of warnings like:

/home/igor/m/tm/js/src/jsinterp.cpp: In function ‘bool js::Interpret(JSContext*)’:
/home/igor/m/tm/js/src/jsinterp.cpp:4215: warning: suggest parentheses around ‘&&’ within ‘||’


/home/igor/m/tm/js/src/jsobj.cpp: In member function ‘bool JSObject::ensureInstanceReservedSlots(JSContext*, size_t)’:
/home/igor/m/tm/js/src/jsobj.cpp:3586: warning: suggest parentheses around ‘&&’ within ‘||’
jsopcode.cpp
Initial performance data is very promising. This patch wins where we expected it to.

One exception: there's a large regression in v8-regexp and ss-unpack-code (could be regex related as well). Despite this problem, the patch is still a win on both benchmarks.
I'm afk a lot today and part of tomorrow but can probably fix any perf regressions in v8-regexp and ss-unpack-code -- anyone have time to profile them, please report results here. Thanks,

/be
http://hg.mozilla.org/tracemonkey/rev/f3e58c264932 - followup to fix Linux link error and warnings
(In reply to comment #144)
> One exception: there's a large regression in v8-regexp and ss-unpack-code
> (could be regex related as well). Despite this problem, the patch is still a
> win on both benchmarks.

Also did ss-nsieve-bits regress due to this patch?

/be
(In reply to comment #147)
> (In reply to comment #144)
> > One exception: there's a large regression in v8-regexp and ss-unpack-code
> > (could be regex related as well). Despite this problem, the patch is still a
> > win on both benchmarks.
> 
> Also did ss-nsieve-bits regress due to this patch?

Doesn't look like it so far. It bounced upward on the initial checkin, but the followup bustage fixes show it back to where it was.
I compared these two revisions with Cachegrind on 32-bit Linux:

BEFORE
changeset:   51603:eae8350841be
user:        Igor Bukanov <igor@mir2.org>
date:        Thu Aug 12 15:02:51 2010 +0200
summary:     bug 477999 - JS_SuspendRequest should suspend requests from all contexts. r=anygregor,gal

AFTER
changeset:   51607:f3e58c264932
tag:         tip
user:        Igor Bukanov <igor@mir2.org>
date:        Sun Aug 29 23:24:23 2010 +0200
summary:     bug 558451 - followup to fix GCC warnings and link error. "CLOSED TREE"

---------------------------------------------------------------
| millions of instructions executed                           |
| total                        | on-trace (may overestimate)  |
---------------------------------------------------------------
|   110.495   111.894 (0.987x) |    20.794    20.813 (0.999x) | 3d-cube
|    56.930    56.360 (1.010x) |    22.313    22.312 (------) | 3d-morph
|   145.557   145.632 (0.999x) |    21.941    21.621 (1.015x) | 3d-raytrace
|    76.961    57.230 (1.345x) |    17.405    16.516 (1.054x) | access-binary-
|   183.408   181.938 (1.008x) |   103.702   103.701 (------) | access-fannkuc
|    41.342    41.044 (1.007x) |    17.548    17.149 (1.023x) | access-nbody
|    59.977    59.938 (1.001x) |    27.012    27.012 (------) | access-nsieve
|    15.668    15.845 (0.989x) |     2.862     2.862 (------) | bitops-3bit-bi
|    43.280    43.455 (0.996x) |    30.281    30.280 (------) | bitops-bits-in
|    22.745    22.925 (0.992x) |    10.219    10.218 (------) | bitops-bitwise
|    71.603    71.362 (1.003x) |    39.453    39.452 (------) | bitops-nsieve-
|    42.350    42.475 (0.997x) |    26.654    26.653 (------) | controlflow-re
|    46.901    47.156 (0.995x) |     5.654     5.618 (1.006x) | crypto-md5
|    31.198    31.452 (0.992x) |     7.107     7.091 (1.002x) | crypto-sha1
|   114.470   110.842 (1.033x) |    11.936    11.818 (1.010x) | date-format-to
|    85.821    85.315 (1.006x) |    10.611    10.303 (1.030x) | date-format-xp
|    52.565    52.837 (0.995x) |    27.614    27.613 (------) | math-cordic
|    28.958    29.464 (0.983x) |     6.165     6.413 (0.961x) | math-partial-s
|    28.451    28.631 (0.994x) |    10.284    10.284 (------) | math-spectral-
|    58.162    58.487 (0.994x) |    34.590    34.590 (------) | regexp-dna
|    40.239    40.243 (------) |    10.208     9.896 (1.032x) | string-base64
|   122.826   122.280 (1.004x) |    24.543    24.509 (1.001x) | string-fasta
|   167.267   163.011 (1.026x) |    11.146    10.968 (1.016x) | string-tagclou
|   180.150   180.143 (------) |    10.927    10.830 (1.009x) | string-unpack-
|    51.260    51.232 (1.001x) |     8.451     8.360 (1.011x) | string-validat
-------
|  1878.597  1851.201 (1.015x) |   519.432   516.894 (1.005x) | all


---------------------------------------------------------------
| millions of instructions executed                           |
| total                        | on-trace (may overestimate)  |
---------------------------------------------------------------
|  2102.778  2085.608 (1.008x) |  1087.530  1081.387 (1.006x) | v8-crypto
|  2183.622  1976.169 (1.105x) |  1095.547  1011.251 (1.083x) | v8-deltablue
|  6566.138  5669.497 (1.158x) |   102.037    85.981 (1.187x) | v8-earley-boye
|  2362.584  2136.183 (1.106x) |    90.032    84.010 (1.072x) | v8-raytrace
|  1752.076  2094.375 (0.837x) |   393.222   389.667 (1.009x) | v8-regexp
|  1716.776  1612.225 (1.065x) |  1655.915  1555.345 (1.065x) | v8-richards
|  1362.786  1154.315 (1.181x) |   139.911   134.746 (1.038x) | v8-splay
-------
| 18046.778 16728.383 (1.079x) |  4564.197  4342.389 (1.051x) | all


Conclusions: 
- access-binary-trees saw a big win, as expected.  The detailed profile showed much less malloc activity.

- unpack-code was barely changed, so I think it's worth double-checking if it really has regressed.  I don't see the regression in timings on my Linux box.

- v8-regexp has definitely regressed.  PropertyTable::init() appears to be the main culprit, I'll post detailed profiles shortly.  Bug 581595 (which I was about to land, but I'll now wait some) will probably remove most of the regression as it prevents heaps of property operations from occurring, but it would be good to understand the problem properly before masking it.

- The rest of V8 did very well!
Note that the function-by-function counts don't always match up with the line-by-line counts like you'd expect, due to inlining.  If you squint a bit it'll usually end up making sense, though.
Shark shows 7.6% of the runtime in |js::PropertyTree::sweepShapes| under the |szone_free_definite_size| system library function.
That's a huge speedup for a micro-benchmark I wrote for the GC.
The benchmark creates linked-lists with one million objects and GC's them.
The finalize object phase reduced from 276ms to 40ms!
Depends on: 592001
No longer blocks: 562793
Depends on: 592007
blocking2.0: beta5+ → beta6+
Is there a bug about the need for a non-stub Unlink method for JSObjects as mentioned in the comment in call_trace? Asking because I looked through the dependencies and didn't find it. The CC has always relied on the GC to break edges between the JS engine's objects and I don't understand why we'd want to change that.
(In reply to comment #155)
> Is there a bug about the need for a non-stub Unlink method for JSObjects as
> mentioned in the comment in call_trace? Asking because I looked through the
> dependencies and didn't find it. The CC has always relied on the GC to break
> edges between the JS engine's objects and I don't understand why we'd want to
> change that.

The JS GC does collect cycles that consist only of JS objects.

The problem is that the here involved an IDBRequest retained by a var in an active function, where the function was heavyweight and therefore had a Call object. The Call object used to be lazily populated with properties reflecting vars and args, and what's more, those properties were JSPROP_SHARED, or slotless. Thus nothing while the function was active updated those slots. Only when the function returned did js_PutCallObject copy from stack slots to object slots.

With this bug's patches applied, the CC saw the reference through the Call object slot early, where before it did not. Even though the JS GC can collect a pure JS cycle, this cycle involved XPCOM ref-counting. My fix of "blanking" the callobj slots in call_trace worked. I therefore conclude that the CC needs something it lacks from JS: Unlink for JS objects.

/be
(In reply to comment #156)
> With this bug's patches applied,

(of course I meant without the tryserver-fixes final patch...)

> the CC saw the reference through the Call
> object slot early, where before it did not. Even though the JS GC can collect a
> pure JS cycle, this cycle involved XPCOM ref-counting. My fix of "blanking" the
> callobj slots in call_trace worked. I therefore conclude that the CC needs
> something it lacks from JS: Unlink for JS objects.

What's more, if you run the following in _tests/testing/mochitest on tm tip:

python runtests.py --test-path=dom/indexedDB/test/test_cursors.html --debugger-interactive --debugger=gdb

you get this assertbotch and stack on shutdown:

http://pastebin.mozilla.org/779211

Of course, a full mochitest run succeeds.

I filed this as bug 592352.

/be
(In reply to comment #156)
> The problem is that the here involved an IDBRequest retained by a var in an

Sigh. Tired from my labors. I meant to write "The problem is that the cycle here involved an IDBRequest", with the XPCOM ref-counted nature of that object left implicit. Something is going on that involves XPCOM refcounts, bottom line.

My fix to call_trace was based on analyzing how things worked before this bug's patches up to but not including tryserver-fixes were applied: no live function vars show up in Call object slots, ever. The function has to return before the js_PutCallObject function runs.

The fix of "blanking" callobj slots if the callobj's function is active works. Ergo, CC is tracing through the unblanked slots but never unlinking for want of a non-stub Unlink. Does this make sense?

/be
Peter and I chatted on IRC, and he convinced me it is premature to blame the stub CC Unlink method for JS objects. But something is broken in CC or possibly IDB land. He's taken bug 592352 (thanks!).

/be
Review of tryserver-fixes patch:

>+static void
>+call_trace(JSTracer *trc, JSObject *obj)
>+{
>+    JS_ASSERT(obj->isCall());
>+    JSStackFrame *fp = (JSStackFrame *) obj->getPrivate();
>+    if (fp) {
>+        /*
>+         * FIXME: Hide copies of stack values rooted by fp from the Cycle
>+         * Collector, which currently lacks a non-stub Unlink implementation
>+         * for JS objects (including Call objects), so is unable to collect
>+         * cycles involving Call objects whose frames are active without this
>+         * hiding hack.
>+         */
>+        uintN first = JSSLOT_PRIVATE + CALL_CLASS_RESERVED_SLOTS + 1;
>+        JS_ASSERT(first <= JS_INITIAL_NSLOTS);
>+
>+        uintN count = fp->getFunction()->countArgsAndVars();
>+        uintN fixed = JS_MIN(count, JS_INITIAL_NSLOTS - first);
>+
>+        SetValueRangeToUndefined(&obj->fslots[first], fixed);
>+        SetValueRangeToUndefined(obj->dslots, count - fixed);
>+    }
>+
>+    MaybeMarkGenerator(trc, obj);
>+}

Why aren't these slots empty to begin with?

>-        fun->u.i.names = cx->runtime->emptyFunctionShape;
>+        fun->u.i.names = cx->runtime->emptyCallShape;

I'm uneasy about this change. Why is it necessary?

This certainly implies weakening the basic layout guarantee so as not
to cover JSClass exactly. What exactly is the new guarantee?

In jsscope.h, struct Shape:
>+        /* Prevent unwanted mutation of shared JSFunction::u.i.names nodes. */
>+        FROZEN          = 0x10
>     };

Why is this FROZEN bit necessary? It seems like it is only tested in one place:

In JSObject::getChildProperty:
>     if (inDictionaryMode()) {
>         JS_ASSERT(parent == lastProp);
>+        if (parent->frozen()) {
>+            parent = Shape::newDictionaryList(cx, &lastProp);
>+            if (!parent)
>+                return NULL;
>+            JS_ASSERT(!parent->frozen());
>+        }

...but how can fun->u.i.names ever be in dictionary mode?
blocking2.0: beta6+ → ---
(In reply to comment #160)
> Review of tryserver-fixes patch:
> 
> >+static void
> >+call_trace(JSTracer *trc, JSObject *obj)
> >+{
> >+    JS_ASSERT(obj->isCall());
> >+    JSStackFrame *fp = (JSStackFrame *) obj->getPrivate();
> >+    if (fp) {
> >+        /*
> >+         * FIXME: Hide copies of stack values rooted by fp from the Cycle
> >+         * Collector, which currently lacks a non-stub Unlink implementation
> >+         * for JS objects (including Call objects), so is unable to collect
> >+         * cycles involving Call objects whose frames are active without this
> >+         * hiding hack.
> >+         */
> >+        uintN first = JSSLOT_PRIVATE + CALL_CLASS_RESERVED_SLOTS + 1;
> >+        JS_ASSERT(first <= JS_INITIAL_NSLOTS);
> >+
> >+        uintN count = fp->getFunction()->countArgsAndVars();
> >+        uintN fixed = JS_MIN(count, JS_INITIAL_NSLOTS - first);
> >+
> >+        SetValueRangeToUndefined(&obj->fslots[first], fixed);
> >+        SetValueRangeToUndefined(obj->dslots, count - fixed);
> >+    }
> >+
> >+    MaybeMarkGenerator(trc, obj);
> >+}
> 
> Why aren't these slots empty to begin with?

They are: JSObject::ensureInstanceReservedSlots calls JSObject::allocSlots (JSObject::allocSlots and JSObject::growSlots call SetValueRangeToUndefined).

This is blanking out copies of values from fp's slots to work around a CC and/or IDB bug. See bug 592352.

> >-        fun->u.i.names = cx->runtime->emptyFunctionShape;
> >+        fun->u.i.names = cx->runtime->emptyCallShape;
> 
> I'm uneasy about this change. Why is it necessary?

Notice that emptyCallShape was unused in prior patches. There's nothing to be uneasy about.

> This certainly implies weakening the basic layout guarantee so as not
> to cover JSClass exactly. What exactly is the new guarantee?

Are you thinking fun->u.i.names is a shape of function objects? It is not -- that would be fun->JSObject::lastProp. Rather, fun->u.i.names is the compiler-precomputed shape of all Call objects for that function.

> In jsscope.h, struct Shape:
> >+        /* Prevent unwanted mutation of shared JSFunction::u.i.names nodes. */
> >+        FROZEN          = 0x10
> >     };
> 
> Why is this FROZEN bit necessary?

It's necessary because fun->u.i.names may point to a shared dictionary, where the shares are Call objects referencing it via their JSObject::lastProp members and of course their underlying function object via JSFunction::u.i.names.

The sharing would be unproblematic but for non-ES5-strict eval("var x...") which extends the scope of an active Call object. This must not mutate the common dictionary-list/hash!

> It seems like it is only tested in one place:
> 
> In JSObject::getChildProperty:
> >     if (inDictionaryMode()) {
> >         JS_ASSERT(parent == lastProp);
> >+        if (parent->frozen()) {
> >+            parent = Shape::newDictionaryList(cx, &lastProp);
> >+            if (!parent)
> >+                return NULL;
> >+            JS_ASSERT(!parent->frozen());
> >+        }
> 
> ...but how can fun->u.i.names ever be in dictionary mode?

See JSFunction::addLocal, which you reviewed :-P.

/be
(In reply to comment #161)
> > Why aren't these slots empty to begin with?
> 
> They are: JSObject::ensureInstanceReservedSlots calls JSObject::allocSlots
> (JSObject::allocSlots and JSObject::growSlots call SetValueRangeToUndefined).

Hm, but what populates the slots when fp is not null? I.e. why are they filled with values that can be blanked out during the GC?
(In reply to comment #161)
> The sharing would be unproblematic but for non-ES5-strict eval("var x...")

Not only eval("var x") in non-strict functions -- also nested function sub-statements in our extension supporting that syntactic form. These extend the scope depending on control flow and require a Call object. Blech!

Need let-like function sub-statements yesterday.

/be
(In reply to comment #162)
> (In reply to comment #161)
> > > Why aren't these slots empty to begin with?
> > 
> > They are: JSObject::ensureInstanceReservedSlots calls JSObject::allocSlots
> > (JSObject::allocSlots and JSObject::growSlots call SetValueRangeToUndefined).
> 
> Hm, but what populates the slots when fp is not null? I.e. why are they filled
> with values that can be blanked out during the GC?

Because the compiler-precomputed shapes for args and vars have slots now -- they map the allocated slots (required for any object that might be extended later, or else the emptyShape must be specialized to each Call object's "arity").

/be
Another reason to freeze (JSC uses "pin", I toyed with that but went with the more anti-mutation trope) shapes: to get rid of JSObject::freeslot by computing the next free slot to allocate based on lastProp->slot if !lastProp->table. The table would need to stick around once lastProp->slot was no longer reliable (if an out of order slot were claimed, e.g. a JSCLASS_GLOBAL_FLAGS-reserved slot).

/be
Depends on: 593256
Attachment #470304 - Flags: review?(jorendorff) → review+
Depends on: 595365
http://hg.mozilla.org/mozilla-central/rev/e5958cd4a135
http://hg.mozilla.org/mozilla-central/rev/f3e58c264932 (followup GCC fixage)

/be
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
No longer blocks: 367425
Depends on: 595918
Depends on: 610370
Depends on: 617236
(In reply to comment #141)
> I landed:
> 
> http://hg.mozilla.org/tracemonkey/rev/e5958cd4a135
This changeset is believed to have caused ~0.9MB/tab memory usage increase, for more info see bug 617236.
Depends on: 620316
Depends on: 621991
No longer depends on: 592202
Depends on: 592202
Blocks: 568276
You need to log in before you can comment on or make changes to this bug.