511425 - removal of JSObjectOps.(get|set)RequiredSlot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

With the removal of the liveconnect JSObjectOps.(get|set)RequiredSlot is only defined for native objects. I suggest to remove these methods and simplify code assuming that only native objects can have required slots.

Comment 1

[Igor Bukanov]

Attachment: v1

Here is an untested patch. Besides removal of (get|set)RequiredSlot it also changes SLOT macros to insist that the object is native. This avoids useless OBJ_IS_NATIVE check in OBJ_(GET|SET)_SLOT macros since the fast array, the only non-native object, accesses the slots directly.

Comment 2

[Igor Bukanov]

Attachment: v2

This is what I have submitted to the try server.

Comment 3

[Igor Bukanov]

Attachment: v3

Here is the version that compiles and passes try server. Besides changes from the comment 1 it replaces few more jsobj.cpp macroses with inlined function. To keep patch relatively small I have kept (ST)?OBJ_(GET|SET|CLEAR)_(PROTO|PARENT) unless the patch has to touch the code in any case.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 397096 [details] [diff] [review]
v3

Nice -- but do we need the JS_ALWAYS_INLINE qualifiers on the new JSObject instance methods? I think not, but I defer to experts.

/be

Comment 5

[Jeff Walden [:Waldo]]

Don't think so, excepting evidence that any of them aren't actually inlined and that matters perf-wise.

Comment 6

[Brendan Eich [:brendan]]

Let's lose all the JS_ALWAYS_INLINE noise then. Thanks,

/be

Comment 7

[Igor Bukanov]

Attachment: v4

In the new version I dropped JS_ALWAYS_INLINE or replaced it with the inline keyword.

Comment 8

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/10380ffe4d49

Comment 9

[Brendan Eich [:brendan]]

I've argued against inline qualifier on an instance method defined inside its class. It's redundant and it adds noise.

If you have to forward-declare the instance method but it's inline later on in the .h file (to use later definitions, say), then the inline is a useful clue.

/be

Comment 10

[Jason Orendorff [:jorendorff]]

+/* On failure the function unlocks the object. */
+static bool
+ReservedSlotIndexOK(JSContext *cx, JSObject *obj, JSClass *clasp,
+                    uint32 index, uint32 limit)
+{
+    JS_IS_OBJ_LOCKED(cx, obj);

Is that last line supposed to be an assertion? gcc warns that the statement has no effect.

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Is that last line supposed to be an assertion? gcc warns that the statement has
> no effect.

Thanks for spotting this, I fixed this in the follow up:

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

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/10380ffe4d49

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/042313efb6ac