555631 - Convert STOBJ_* macros to inline functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch removes all STOBJ_* macros:

- Replaces STOBJ_NSLOTS with the new JSObject::nSlotsST().  I put the "ST"
  at the end because it's auxiliary info, "getSlot" is the main part of the
  name.  And STnSlots() would be really ugly.

- Replaces STOBJ_GET_SLOT with the new JSObject::getSlotST().

- Replaces STOBJ_SET_SLOT with the new JSObject::setSlotST().

- Replaces STOBJ_GET_CLASS calls with the pre-existing JSObject::getClass().
  It's interesting that getClass() has no "ST" marker on it, should it?

I confirmed on i386/Linux that the binary size is unchanged and the number
of instructions is all-but-unchanged (within 0.0001%).  Just in case anyone
was worried about GCC correctly inlining...

Blake, sorry for the r? on such an eye-bleeding patch.

Passes JS trace-tests, JS regtests and some xpcshell tests (the ones in netwerk/tests).

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Created an attachment (id=435529) [details]
> patch
> 
> This patch removes all STOBJ_* macros:
> 
> - Replaces STOBJ_NSLOTS with the new JSObject::nSlotsST().  I put the "ST"
>   at the end because it's auxiliary info, "getSlot" is the main part of the
>   name.  And STnSlots() would be really ugly.

Old C hackers use nslots for number of slots, but more newfangled camelCaps method names make me want to get modern and throw in 'um' for numSlots. The lone lowercase 'n' seems lost and lonely.

> - Replaces STOBJ_GET_SLOT with the new JSObject::getSlotST().
> 
> - Replaces STOBJ_SET_SLOT with the new JSObject::setSlotST().

Yay.

> - Replaces STOBJ_GET_CLASS calls with the pre-existing JSObject::getClass().
>   It's interesting that getClass() has no "ST" marker on it, should it?

No. We are not going to change class of objects accessible across threads. And we are not using a roving obj->slots pointer that requires synchronization to access for such MT objects.

BTW, this lock-free class getter macro was another win of fslots, back when it was introduced and class was an fslot. You could argue against the MT access issue even being possible, of course, and we are pushing back on that finally, but the point remains that some fields need to be in the lock-free, non-moving object header for performance.

And even more obviously, we lazy hackers deferred cleaning up too many of the ancient OBJ_GET_CLASS(cx, obj) calls that, with the advent of fslots, no longer used cx at all. This led to BOGUS_CX callsite temporizing, for which your fine patch is the true cure.

/be

Comment 2

[David Mandelin [:dmandelin]]

How about just |getSlot| and add "MT" for the other ones? I think the ST access functions are used almost all the time now, and the MT ones rarely.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> How about just |getSlot| and add "MT" for the other ones? I think the ST access
> functions are used almost all the time now, and the MT ones rarely.

I'm happy to change the names to whatever people want, but I won't do it unless someone gives a definitive "do it" recommendation.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch v2

Changes nSlotsST() to numSlotsST() as per Brendan's suggestion.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #2)
> How about just |getSlot| and add "MT" for the other ones? I think the ST access
> functions are used almost all the time now, and the MT ones rarely.

This is a good point. The plan of record is to move MT object locking under a non-native JSObjectOps impl, as noted often lately. So the name tax should apply to the MT case, which should then move out of the common .h files.

/be

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
>
> So the name tax should
> apply to the MT case, which should then move out of the common .h files.

Which names would have MT?  I see the following *OBJ_* macros in jsobj.h:

- OBJ_IS_NATIVE
- OBJ_CHECK_SLOT
- OBJ_GET_SLOT
- OBJ_SET_SLOT
- LOCKED_OBJ_GET_SLOT
- LOCKED_OBJ_SET_SLOT
- OBJ_GET_CLASS
- OBJ_BLOCK_COUNT
- OBJ_BLOCK_DEPTH
- OBJ_SET_BLOCK_DEPTH

Any others I've missed?

Once I get a clear answer to that question I'll remove the ST from the new names and change the relevant *OBJ_* macros to "MT" functions as well.  (Nb: I'll function-ify the remaining macros at some point, but not in this patch.)

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Which names would have MT?  I see the following *OBJ_* macros in jsobj.h:

That should be only OBJ_GET_SLOT/OBJ_SET_SLOT.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch v3

New patch:

- Renames numSlotsST() as numSlots(), and {g,s}etSlotST() as {g,s}etSlot(),
  as per the above suggestions.

- Converts OBJ_{GET,SET}_SLOT to JSObject::{g,s}etSlotMT().  They had to be
  put into jsobjinlines.h because they needed to see things in jsscope.h.
  jsobjinlines.h then had to be #included in a few more .cpp files.

- Changes getSlot() to no longer return a reference;  getSlotRef() now does
  that.  This was partly because getSlot() now needs to match
  js_GetSlotThreadSafe() which doesn't return a reference, and partly
  because it seems nice to have a non-reference version which is const
  w.r.t. the JSObject.

If anyone else wants to steal the review from Blake I'm sure he won't mind :)

Comment 9

[Andreas Gal :gal]

Comment on attachment 435807 [details] [diff] [review]
patch v3

 
>     if (!scope->table) {
>-        if (slot < STOBJ_NSLOTS(obj) && !OBJ_GET_CLASS(cx, obj)->reserveSlots) {
>-            JS_ASSERT(JSVAL_IS_VOID(STOBJ_GET_SLOT(obj, scope->freeslot)));
>+        if (slot < obj->numSlots() && !OBJ_GET_CLASS(cx, obj)->reserveSlots) {
>+            JS_ASSERT(JSVAL_IS_VOID(obj->getSlot(scope->freeslot)));

obj->getClass()->reserveSlots

>         if (arg < fp->argc) {
>             if (argsobj) {
>-                jsval v = OBJ_GET_SLOT(cx, argsobj, JSSLOT_ARGS_COPY_START+arg);
>+                jsval v = argsobj->getSlotMT(cx, JSSLOT_ARGS_COPY_START+arg);

Brendan, can argument objects actually be shared? We should just do the ST version here IMO.

>         uintN arg = uintN(JSVAL_TO_INT(idval));
>         if (arg < GetArgsLength(obj))
>-            OBJ_SET_SLOT(cx, obj, JSSLOT_ARGS_COPY_START + arg, JSVAL_HOLE);
>+            obj->setSlotMT(cx, JSSLOT_ARGS_COPY_START + arg, JSVAL_HOLE);

Dito here.

>             } else {
>-                jsval v = OBJ_GET_SLOT(cx, obj, JSSLOT_ARGS_COPY_START + arg);
>+                jsval v = obj->getSlotMT(cx, JSSLOT_ARGS_COPY_START + arg);

And here.

>         if (arg < GetArgsLength(obj) &&
>-            OBJ_GET_SLOT(cx, obj, JSSLOT_ARGS_COPY_START + arg) != JSVAL_HOLE) {
>+            obj->getSlotMT(cx, JSSLOT_ARGS_COPY_START + arg) != JSVAL_HOLE) {

And here.

> #define OBJ_BLOCK_COUNT(cx,obj)                                               \
>     (OBJ_SCOPE(OBJ_IS_CLONED_BLOCK(obj) ? obj->getProto() : obj)->entryCount)
> #define OBJ_BLOCK_DEPTH(cx,obj)                                               \
>-    JSVAL_TO_INT(STOBJ_GET_SLOT(obj, JSSLOT_BLOCK_DEPTH))
>+    JSVAL_TO_INT(obj->getSlot(JSSLOT_BLOCK_DEPTH))
> #define OBJ_SET_BLOCK_DEPTH(cx,obj,depth)                                     \
>-    STOBJ_SET_SLOT(obj, JSSLOT_BLOCK_DEPTH, INT_TO_JSVAL(depth))
>+    obj->setSlot(JSSLOT_BLOCK_DEPTH, INT_TO_JSVAL(depth))

If you want to de-macro-ize these, you have my support :)

>-    vp = &STOBJ_GET_SLOT(obj, slot);
>+    vp = &obj->getSlotRef(slot);

I think I actually would prefer getSlotAddr() returning a jsval *. References as return value are always mind boggling, but its up to you. r=me either way.

Comment 10

[Andreas Gal :gal]

Note: (ST)OBJ_GET/SET_SLOT are in jsobj.h, so I assume its not a public api and its ok to remove it.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> >+        if (slot < obj->numSlots() && !OBJ_GET_CLASS(cx, > 
> obj->getClass()->reserveSlots
> 
> > #define OBJ_BLOCK_COUNT(cx,obj)                                               \
> >     (OBJ_SCOPE(OBJ_IS_CLONED_BLOCK(obj) ? obj->getProto() : obj)->entryCount)
> > #define OBJ_BLOCK_DEPTH(cx,obj)                                               \
> >-    JSVAL_TO_INT(STOBJ_GET_SLOT(obj, JSSLOT_BLOCK_DEPTH))
> >+    JSVAL_TO_INT(obj->getSlot(JSSLOT_BLOCK_DEPTH))
> > #define OBJ_SET_BLOCK_DEPTH(cx,obj,depth)                                     \
> >-    STOBJ_SET_SLOT(obj, JSSLOT_BLOCK_DEPTH, INT_TO_JSVAL(depth))
> >+    obj->setSlot(JSSLOT_BLOCK_DEPTH, INT_TO_JSVAL(depth))
> 
> If you want to de-macro-ize these, you have my support :)

I plan to do all these, just not in this patch :)

 
> >-    vp = &STOBJ_GET_SLOT(obj, slot);
> >+    vp = &obj->getSlotRef(slot);
> 
> I think I actually would prefer getSlotAddr() returning a jsval *. References
> as return value are always mind boggling, but its up to you. r=me either way.

I agree!  I tried doing it that way but jstracer.cpp uses references quite a bit, and once you start using references it's hard to stop.  So I went with the easier option.

Comment 12

[Igor Bukanov]

(In reply to comment #8)
> - Converts OBJ_{GET,SET}_SLOT to JSObject::{g,s}etSlotMT().

IMO lockAndGetSlot/lockAndSetSlot would be much more descriptive names.

(In reply to comment #9)
> can argument objects actually be shared? We should just do the ST version here IMO.

This should go to the not yet landed patch for the bug 554626 avoiding changing semantics in this bug.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #9)
> > can argument objects actually be shared? We should just do the ST version here IMO.
> 
> This should go to the not yet landed patch for the bug 554626 avoiding changing
> semantics in this bug.

I landed 554626's patch just now.

/be

Comment 14

[Andreas Gal :gal]

Yeah, the comment was meant to be a question/note for brendan only, not to be fixed in this patch.

Comment 15

[Nicholas Nethercote [inactive]]

When I land this tomorrow I'll use Igor's suggestion of lockAnd{Get,Set}Slot() unless anyone objects.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> When I land this tomorrow I'll use Igor's suggestion of lockAnd{Get,Set}Slot()
> unless anyone objects.

I'm mildly against -- do these names not imply lock without unlock after? It is not a big deal if these move to a non-native object-ops impl, but how about

lockedGetSlot
lockedSetSlot

Hmm, no, we used LOCKED_OBJ_[GS]ET_SLOT to mean caller already locked. Rats, how about locking[GS]etSlot? But is this really helpful, compared to a big ugly MT suffix? I think not.

/be

Comment 17

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/96e8ea26998f

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to comment #16)
> (In reply to comment #15)
> > When I land this tomorrow I'll use Igor's suggestion of lockAnd{Get,Set}Slot()
> > unless anyone objects.
> 
> I'm mildly against

Ah, crap, I missed your comment before I landed.  I'll revert to the MT suffix in another OBJ_* cleanup patch.

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to comment #18)
>
> I'll revert to the MT suffix in another OBJ_* cleanup patch.

Bug 556353.

Comment 20

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/mozilla-central/rev/96e8ea26998f