555631 - Convert STOBJ_* macros to inline functions

Assignee

Description

•

13 years ago

Attached patch patch (obsolete) — Details — Splinter Review

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

Attachment #435529 - Flags: review?(mrbkap)

Brendan Eich [:brendan]

Comment 1

•

13 years ago

(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

David Mandelin [:dmandelin]

Comment 2

•

13 years ago

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.

Nicholas Nethercote [inactive]

Assignee

Comment 3

•

13 years ago

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

Nicholas Nethercote [inactive]

Assignee

Comment 4

•

13 years ago

Attached patch patch v2 (obsolete) — Details — Splinter Review

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

Attachment #435529 - Attachment is obsolete: true

Attachment #435668 - Flags: review?(mrbkap)

Attachment #435529 - Flags: review?(mrbkap)

Brendan Eich [:brendan]

Comment 5

•

13 years ago

(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

Nicholas Nethercote [inactive]

Assignee

Comment 6

•

13 years ago

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

Blocks: 402259

Igor Bukanov

Comment 7

•

13 years ago

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

Nicholas Nethercote [inactive]

Assignee

Comment 8

•

13 years ago

Attached patch patch v3 — Details — Splinter Review

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 :)

Attachment #435668 - Attachment is obsolete: true

Attachment #435807 - Flags: review?(mrbkap)

Attachment #435668 - Flags: review?(mrbkap)

Andreas Gal :gal

Comment 9

•

13 years ago

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.

Attachment #435807 - Flags: review?(mrbkap) → review+

Andreas Gal :gal

Comment 10

•

13 years ago

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

Nicholas Nethercote [inactive]

Assignee

Comment 11

•

13 years ago

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

Igor Bukanov

Comment 12

•

13 years ago

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

Brendan Eich [:brendan]

Comment 13

•

13 years ago

(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

Andreas Gal :gal

Comment 14

•

13 years ago

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

Nicholas Nethercote [inactive]

Assignee

Comment 15

•

13 years ago

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

Brendan Eich [:brendan]

Comment 16

•

13 years ago

(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

Nicholas Nethercote [inactive]

Assignee

Comment 17

•

13 years ago

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

Whiteboard: fixed-in-tracemonkey

Nicholas Nethercote [inactive]

Assignee

Comment 18

•

13 years ago

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

Nicholas Nethercote [inactive]

Assignee

Comment 19

•

13 years ago

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

Bug 556353.

Nicholas Nethercote [inactive]

Assignee

Comment 20

•

13 years ago

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

Status: ASSIGNED → RESOLVED

Closed: 13 years ago

Resolution: --- → FIXED

patch 13 years ago Nicholas Nethercote [inactive] 122.97 KB, patch		Details \| Diff \| Splinter Review
patch v2 13 years ago Nicholas Nethercote [inactive] 123.03 KB, patch		Details \| Diff \| Splinter Review
patch v3 13 years ago Nicholas Nethercote [inactive] 131.99 KB, patch	gal : review+	Details \| Diff \| Splinter Review

Bugzilla

Quick Search

Convert STOBJ_* macros to inline functions

Categories

(Core :: JavaScript Engine, defect)

Tracking

()

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Crash Data

Security

(public)

User Story

Attachments

(1 file, 2 obsolete files)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15

Comment 16

Comment 17

Comment 18

Comment 19

Comment 20