935855 - Investigate having the jit read JSObject* directly out of C++ objects sometimes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We may want this for window's "document" member, as well as ImageData.data.

I wrote up a version of this that just calls ExposeObjectToActiveJS and another version that inlines the barrier/nursery bits but keeps calls for the GCThingIsMarkedGray/UnmarkGrayGCThingRecursively stuff, and a third version that inlines all of it except the UnmarkGrayGCThingRecursively bits (though this version is not quite correct; see below).

Times, if I'm measuring this correctly, on my hardware:

Normal DOM getter for ImageData.data: 8ns
JIT fast path, calls ExposeObjectToActiveJS: 4.5ns
With the barriering inlined: 4.2ns
With GCThingIsMarkedGray inlined: 2.6ns
With all the barrier/etc logic removed: 0.7ns

The one caveat is that inlining GCThingIsMarkedGray requires doing this in jitcode:

  uintptr_t(1) << (bit % nbits)

and I see no way to do that in our macro assembler (I can shift a register by a 32-bit immediate, but I can't shift by the value stored in a register).  Am I just missing something?

Also, all variants of the JIT fast path assume the object's wrapper and the stored JSObject* are in the same compartment.  There is no attempt to enforce this.  That's certainly part of the speedup.

Finally, I have no particular confidence that my barriering jitcode is correct.... certainly it doesn't quite match what ExposeGCThingToActiveJS does, in that we still check for gray bits even if the barrier stuff triggered.  We'd have to have a way for patchableCallPreBarrier to not bind &done and instead return it to avoid that.

In any case, the main question is whether this is worth doing and if so which of the stages above we want to do...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a [IsMember] WebIDL annotation to bindings.

We want this no matter what, since it lets us avoid Xray gunk for ImageData.data

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store isMember information in jitinfo.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use isMember information in ion codegen.

This is with ExposeObjectToActiveJS called via callWithABI

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: continued; try for faster barriering.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: continued; inline GCThingIsMarkedGray.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Use isMember information in ion codegen.

Missed some hackery that should be removed....

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #0)
> The one caveat is that inlining GCThingIsMarkedGray requires doing this in
> jitcode:
> 
>   uintptr_t(1) << (bit % nbits)
> 
> and I see no way to do that in our macro assembler (I can shift a register
> by a 32-bit immediate, but I can't shift by the value stored in a register).
> Am I just missing something?

It's an x86 quirk: you can shift by the value stored in a register, but that register MUST be ecx. We could add something to the macro-assembler though, and assert on x86/x64 that the rhs register == ecx. Then we can ask the register allocator to reserve ecx as temp, or push/pop ecx before using it...

How many instructions do you have to emit to inline this? We could also generate a small shared code stub that we store in JitRuntime and can just call for every member access.

> In any case, the main question is whether this is worth doing and if so
> which of the stages above we want to do...

Does it noticeably improve non-micro benchmarks? I think you're in the best position to decide if it's worth it; you know where we can use it and what the implementation will look like.

Just calling ExposeObjectToActiveJS seems to make us almost 2x faster, maybe that's a good first step?

Comment 8

[Jan de Mooij [:jandem]]

Looking at the code, it doesn't seem too bad though: lots of bit twiddling but it's fairly straight-forward and I don't expect this to change often.

It would still be nice to move the inlined ExposeObjectToActiveJS code to its own IonCode stub though so that we can just call that for every LGetDOMMember instruction. It's an extra call + ret instruction, but it avoids code bloat. Then if the call to UnmarkGrayGCThingRecursively is really cold, we can stop marking LGetDOMMember as call instruction and just do the spilling/restoring ourselves before/after the callWithABI.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> How many instructions do you have to emit to inline this?

For the fully inlined version, JIT Inspector says:

[GetDOMMember]
movq       0x20(%rcx), %rax
shlq       $1, %rax
movq       0x18(%rax), %rax
jmp        ((1967))
push       %rdx
leaq       0x18(%rax), %rdx
call       ((1977))
pop        %rdx
jmp        ((1983))
.balign 8, 0xf4   # hlt
##link     ((1983)) jumps to ((1984))
##link     ((1967)) jumps to ((1984))
movq       %rax, %rsi
movq       $-1048576, %rdx
andq       %rdx, %rsi
movl       $0xfc0b0, %edx
orq        %rdx, %rsi
movq       %rax, %rdx
movl       $0xfffff, %ecx
andq       %rcx, %rdx
shrq       $3, %rdx
addq       $0x1, %rdx
movq       %rdx, %rcx
shrq       $6, %rdx
andq       $0x3f, %rcx
movq       0(%rsi,%rdx,8), %rsi
movl       $0x1, %edx
andq       %rsi, %rdx
testq      %rdx, %rdx
je         ((2056))
push       %rax
movq       %rax, %rdi
xorl       %esi, %esi
movq       %rsp, %rdx
andq       $0xfffffff0, %rsp
push       %rdx
subq       $0x8, %rsp
movabsq    $0x1013e3690, %rax
call       *%rax
addq       $0x8, %rsp
pop        %rsp
pop        %rax
##link     ((2056)) jumps to ((2092))

(but that's still missing the 1 << (bit % nbits) stuff, whatever that will end up looking like).

For comparison, the minimal version that just does a callWithABI to ExposeObjectToActiveJS looks like this:

[GetDOMMember]
movq       0x20(%rdx), %rdi
shlq       $1, %rdi
movq       0x18(%rdi), %rdi
push       %rdi
movq       %rsp, %rsi
andq       $0xfffffff0, %rsp
push       %rsi
subq       $0x8, %rsp
movabsq    $0x1016001d0, %rax
call       *%rax
addq       $0x8, %rsp
pop        %rsp
pop        %rax


> It would still be nice to move the inlined ExposeObjectToActiveJS code to its own IonCode
> stub

I suspect that you're right here.  I just don't know how to do it yet.  Happy to have someone pick that part up.  ;)

> Then if the call to UnmarkGrayGCThingRecursively is really cold

It should be, as far as I know, yes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Does it noticeably improve non-micro benchmarks?

The main motivation for me finally doing this was bug 935283.  Right now, "document" is an own value property on Window; changing it to a getter definitely significantly slowed down things (several % regressions on jQuery performance).  But that getter currently takes about 30ns.  Presumably we'd get that down to 10 or so once we convert Window to WebIDL.  But that's still slower than the 0.7 we have right now.  So the real question is how we avoid performance regressions when we go back to having document be an accessor property, and this bug was one possible answer, with the goal being to approximate the current slot read as closely as possible, on the assumption that then we definitely won't regress stuff.  I just started with ImageData.data because it's the one other place I know offhand where we want this and performance matters and I could test it easily.

Olli, Bill, are there things we could do that would allow us to skip the barrier and or check-for-gray bits?  For example, is there something the C++ end could do to ensure that the member ("document", say) is never gray when the actual object (window, say) is black, at which point we could presumably skip all the gray checking in the jitcode here?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > How many instructions do you have to emit to inline this?
> 
> For the fully inlined version, JIT Inspector says:
> 
> [GetDOMMember]
> ...
> movq       $-1048576, %rdx
> andq       %rdx, %rsi

This should be andq $-1048576, %rsi.

> movl       $0xfc0b0, %edx
> orq        %rdx, %rsi

This should be orq $0xfc0b0, %rsi.

> movq       %rax, %rdx
> movl       $0xfffff, %ecx
> andq       %rcx, %rdx

This should be andq $0xffffff, %rdx.

> shrq       $3, %rdx
> addq       $0x1, %rdx
> movq       %rdx, %rcx
> shrq       $6, %rdx
> andq       $0x3f, %rcx
> movq       0(%rsi,%rdx,8), %rsi
> movl       $0x1, %edx
> andq       %rsi, %rdx
> testq      %rdx, %rdx
> je         ((2056))

We ought to be emitting:

testq $0x1, %rsi
je LABEL

for this (assuming %rdx isn't being used in the path at LABEL).

Is there an easy way to tell which bits of the JIT are producing these parts of the code?  (Point still stands about this being too slow, of course.)

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Olli, Bill, are there things we could do that would allow us to skip the
> barrier and or check-for-gray bits?  For example, is there something the C++
> end could do to ensure that the member ("document", say) is never gray when
> the actual object (window, say) is black, at which point we could presumably
> skip all the gray checking in the jitcode here?

Olli or Andrew know much more about this than I do. It sounds like something we ought to be able to do, though, if we're not doing it already. I think it would be an improvement to the cycle collector as well (fewer gray objects to scan).

If that doesn't pan out, I have another idea for the gray bits. Most mark bits are not gray. At the end of a GC, we might be able to set a special flag in every arena that has something gray in it. This would have good cache locality, so it would probably be quite fast. I expect that most arenas would end up not having this flag set. So we could check that flag in the fast path, rather than the actual mark bit. If the flag is true, we would take an out-of-line path that would do what we do now.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> This should be andq $-1048576, %rsi.

There is no such thing on x86-64, afaict.  Keep in mind that the constant there is a 64-bit one in this case (it looks negative, but it's actually just very large), but immediates can only be 32-bit.

> This should be orq $0xfc0b0, %rsi.

This one could be if I added some static asserts about the value fitting in 32 bits, yes.  Good catch.  I was being too conservative because of the issue above.

> This should be andq $0xffffff, %rdx.

Yes, with some asserts about it fitting in 32 bits.

> We ought to be emitting:

Don't forget that there's a missing left-shift of edx by rcx there because I have no way to emit it right now.

> Is there an easy way to tell which bits of the JIT are producing these parts of the
> code? 

It's all explicitly in the patch; this is all the manual bitslinging the patch is doing.  For example, this bit:

> movq       $-1048576, %rdx
> andq       %rdx, %rsi

is this part of the patch:

+    // addr &= ~js::gc::ChunkMask.  Make sure to not truncate the
+    // immediate to 32 bits!
+    masm.movePtr(ImmWord(~js::gc::ChunkMask), scratch);
+    masm.andPtr(scratch, arg2Reg);

Comment 14

[Boris Zbarsky [:bzbarsky]]

Andrew, see comment 10 and comment 12?

I agree that if we can just end up with a fast common path putting all the complexity off on a out of line call that should work wonderfully.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> > This should be andq $-1048576, %rsi.
> 
> There is no such thing on x86-64, afaict.  Keep in mind that the constant
> there is a 64-bit one in this case (it looks negative, but it's actually
> just very large), but immediates can only be 32-bit.

and accepts sign-extended 32-bit immediates (int32_t); I think this one qualifies.

> It's all explicitly in the patch; this is all the manual bitslinging the
> patch is doing.

My mistake, I read the bugmail too quickly and didn't bother to look at the wider context of the bug.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Well during black bit propagation we do unmark gray, but that is rather manual and happens rarely.
Here we need something where when the wrapper for window is unmarked, also the wrapper for document
is unmarked.
Can we have some hidden property on window wrapper which points to document wrapper so that unmarking knows to go through it too when it deals with the wrapper for window?

Comment 17

[Andrew McCreight [:mccr8]]

> For example, is there something the C++ end could do to ensure that the member ("document", say) is never gray when the actual object (window, say) is black

I don't really understand what the situation you are describing is.  The "actual object" here is the reflector for the nsGlobalWindow, and the "member" is the reflector for the mDoc of the window?  If the window reflector has a pointer to the document's reflector, then you already have this invariant: black-gray edges are verboten.

I guess that isn't the case though?  From the GC's perspective, maybe whatever other thing owns the window's reflector, and then separately, the document's reflector is gray-rooted by the DOM, and the knowledge that the window's reflector indirectly owns the document's reflector is only in C++.  Maybe the tracer for the window reflector could say that it has the document reflector as a child and then everything will just magically work?  I'm not sure how weird that will get.

Comment 18

[Andrew McCreight [:mccr8]]

"magically work" meaning we will have the invariant that a black window reflector implies a black document reflector, so if JS is operating on the window reflector it can retrieve the document reflector without unmarkgray.  I'm not sure if that helps with GGC stuff, but I think the idea there was already to operate under the assumption that document and window reflectors will be allocated tenured (?!?) so you might be able to avoid some of that goop too.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I guess that isn't the case though?

Right.  The thing with the pointer to mDoc's reflector would be the nsGlobalWindow.

We could give the window reflector a trace hook that traces the document reflector in the nsGlobalWindow, yes.

As for ggc, I agree that if we limit this to document we could skip it all, but I'd still like to see if I can make it work for imagedata.  Of course we could add more stuff to jitinfo to indicate whether we're guaranteed tenured, I guess...

Comment 20

[Boris Zbarsky [:bzbarsky]]

> and accepts sign-extended 32-bit immediates (int32_t); I think this one qualifies.

Hmm.  So we have a 64-bit constant that doesn't fit in 32 bits but which happens to look like a signed integer that _does_ fit in 32 bits (that is, it's very big; if it were smaller then this property would not hold).  I guess we can add some static asserts to that effect and carefully cast to int64_t before passing to the int32_t argument and ignore the undefined behavior issues...

Comment 21

[Eric Faust [:efaust]]

Comment on attachment 828496 [details] [diff] [review]
part 2.  Store isMember information in jitinfo.

Review of attachment 828496 [details] [diff] [review]:
-----------------------------------------------------------------

Codegen changes to jitinfo look good. r=me.

::: dom/bindings/Codegen.py
@@ +6022,5 @@
>                  "  %s,  /* isInfallible. False in setters. */\n"
>                  "  %s,  /* isConstant. Only relevant for getters. */\n"
>                  "  %s,  /* isPure.  Only relevant for getters. */\n"
> +                "  %s,  /* isMember.  Only relevant for getters. */\n"
> +                "  %s,  /* Offset of the member, if isMember. */\n"

nit: Can we add some "0 otherwise" here? I realize we have a space constraint, but it would be good to note. "else 0", maybe?

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 828496 [details] [diff] [review]
part 2.  Store isMember information in jitinfo.

Review of attachment 828496 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/ImageData.h
@@ +61,5 @@
>    JSObject* WrapObject(JSContext* cx, JS::Handle<JSObject*> scope);
>  
> +  static const size_t DataOffset()
> +  {
> +    return offsetof(ImageData, mData);

Or we could just hardcode this in codegen as |offsetof(nativeType, "m" + nativeMemberName)|.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Or we could just hardcode this in codegen as |offsetof(nativeType, "m"

Doesn't this require the member of the nativeType to be public?

Comment 24

[Boris Zbarsky [:bzbarsky]]

So I was thinking.  The other possible option here to get around issues like grayness and worries about compartments is to just store the value in a reserved slot and read it from there instead of from a member of the C++ object.  Basically Olli's "invisible property" suggestion.

We could maybe even autogenerate code to stash it in there, to make wrapping such objects not insane.

The drawbacks are that we have to store it boxed (which is no worse than a normal JS property) and that we have to have free reserved slots to do this (which is not the case for Window right now, though maybe we can easily add one?).

Thoughts?  It seems a bit less flexible than what I have here so far, but also seems like it would generate vastly better jitcode.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

If we could do that in generated code, I definitely favor that.
The less we add JS* stuff to manually written C++, the better.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure we could.  Just have the generated wrapping code call the normal getter when it's done and store the result in the reserved slot.

If we think we'll be able to get at least one more reserved slot on Window, I'm happy to try that out.

Comment 27

[Peter Van der Beken [:peterv]]

Yeah, that'd be nicer. A little sad that we'd store two copies of the same value.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Actually, there is one more complication to the reserved slot approach.  When the value changes the C++ code has to reset the reserved slot.  We could codegen helpers for that too, though, I think.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Also, I'm going to not allow this on non-leaf interfaces for now, just for simplicity.  That won't affect any of our use cases.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

In the future, it might be nice if proxies could have "reserved C++ slots". These would be slots that would hold untagged pointers to JSObjects (and maybe other GC thing types, based on some sort of type descriptor in the proxy). Both C++ code and JS code could access them quickly. The GC would trace them as it now traces reserved slots. This would have to wait until proxies have their own classes, probably. Does it sound useful?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Well, ideally proxies and normal objects would not differ in this regard.

For my purposes here, right now there are no proxies I need this stuff for, so I'm not blocked on the proxy situation.

Comment 32

[Jason Orendorff [:jorendorff]]

(In reply to Boris Zbarsky [:bz] from comment #26)
> If we think we'll be able to get at least one more reserved slot on Window,
> I'm happy to try that out.

The customer is always right.

It should be as simple as bumping GlobalObject::APPLICATION_SLOTS in js/src/vm/GlobalObject.h. The magic of constants.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Ah, awesome.  ;)

So I have a prototype of this, and it's nice and fast.  Just cleaning it up now.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a [StoreInSlot] WebIDL annotation to bindings.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Compute the right slot indices to use for members we want to store in slots and save then in the data model the WebIDL parser outputs.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Store isSlot information in jitinfo.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Codegen a function that updates all the members-in-reserved-slots values for an interface and call it when we finish wrapping an object.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Use our members-in-slots information in ion codegen.

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 831012 [details] [diff] [review]
part 5.  Use our members-in-slots information in ion codegen.

Review of attachment 831012 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +7320,5 @@
>  
>  bool
> +CodeGenerator::visitGetDOMMember(LGetDOMMember *ins)
> +{
> +    // It's simple to duplicate visitLoadFixedSlotV here than it is to try to

simpler?

::: js/src/jit/IonBuilder.cpp
@@ +8374,5 @@
> +            // We can't use MLoadFixedSlot here because it might not have the
> +            // right aliasing behavior; we want to alias DOM setters.
> +            get = MGetDOMMember::New(jitinfo, obj, guard);
> +        } else
> +            get = MGetDOMProperty::New(jitinfo, obj, guard);

{}

Comment 40

[Boris Zbarsky [:bzbarsky]]

> simpler?

Yes, thank you.

> {}

Afaik, not in jseng.

Comment 41

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #40)
> Afaik, not in jseng.

No, it should be braced: "However, if either the consequent or alternate is a block of multiple statements, braces are used on both." (from https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style )

Comment 42

[Andrew McCreight [:mccr8]]

Though I guess IonMonkey has its own style which I am unfamiliar with...

Comment 43

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #42)
> Though I guess IonMonkey has its own style which I am unfamiliar with...

Same applies to IonMonkey code.

(AFAIK IonMonkey style == SpiderMonkey style, except "//" comments and maybe different naming for class/struct members, but not sure whether the SM style guide says anything about that. Both of these also start to show up in non-Ion code though.)

Comment 44

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up patch

Comment 45

[Eric Faust [:efaust]]

Comment on attachment 830997 [details] [diff] [review]
part 2.  Store isSlot information in jitinfo.

Review of attachment 830997 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, after reading the rollup patch. r=me

::: dom/bindings/Codegen.py
@@ +192,5 @@
>          return ""
>      def define(self):
>          traceHook = TRACE_HOOK_NAME if self.descriptor.customTrace else 'nullptr'
>          callHook = LEGACYCALLER_HOOK_NAME if self.descriptor.operations["LegacyCaller"] else 'nullptr'
> +        slotCount = INSTANCE_RESERVED_SLOTS

I see that this line changes in part 3. It must do so, else the scheme makes no sense.

Comment 46

[Eric Faust [:efaust]]

Comment on attachment 831012 [details] [diff] [review]
part 5.  Use our members-in-slots information in ion codegen.

Review of attachment 831012 [details] [diff] [review]:
-----------------------------------------------------------------

Very nicely done. This system is a pain to navigate. I'm also really happy to see that the gray object unmarking stuff went away because we didn't need it. \o/. r=me with single nit addresed.

::: js/src/jit/CodeGenerator.cpp
@@ +7324,5 @@
> +    // It's simple to duplicate visitLoadFixedSlotV here than it is to try to
> +    // use an LLoadFixedSlotV or some subclass of it for this case: that would
> +    // require us to have MGetDOMMember inherit from MLoadFixedSlot, and then
> +    // we'd have to duplicate a bunch of stuff we now get for free from
> +    // MGetDOMProperty.

That's fine. GetDOMMember is much more the logical child of GetDOMProperty and LoadFixedSlotV, though they do similar work. masm.loadValue is a fine place to leave the abtraction.

::: js/src/jit/IonBuilder.cpp
@@ +8374,5 @@
> +            // We can't use MLoadFixedSlot here because it might not have the
> +            // right aliasing behavior; we want to alias DOM setters.
> +            get = MGetDOMMember::New(jitinfo, obj, guard);
> +        } else
> +            get = MGetDOMProperty::New(jitinfo, obj, guard);

nit: Ms2ger is right. The braces in js/ should match in both arms, regardless of the length of either one.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> nit: Ms2ger is right.

The world is ending.  ;)

I'll add the braces.  Thanks for the review!

Comment 48

[Peter Van der Beken [:peterv]]

Comment on attachment 830996 [details] [diff] [review]
part 3.  Compute the right slot indices to use for members we want to store in slots and save then in the data model the WebIDL parser outputs.

Review of attachment 830996 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +6046,5 @@
>              getterinfal = getterinfal and infallibleForMember(self.member, self.member.type, self.descriptor)
>              isInSlot = self.member.getExtendedAttribute("StoreInSlot")
>              if isInSlot:
>                  slotIndex = INSTANCE_RESERVED_SLOTS + self.member.slotIndex;
> +                if slotIndex >= 16 : # JS engine currently allows 16 fixed slots

FIXED_SLOTS_MAX seems to be 31? It'd be nice to assert this in the C++ somewhere.

Comment 49

[Peter Van der Beken [:peterv]]

Comment on attachment 831011 [details] [diff] [review]
part 4.  Codegen a function that updates all the members-in-reserved-slots values for an interface and call it when we finish wrapping an object.

Review of attachment 831011 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +8361,5 @@
>                  cgThings.append(CGConstructorEnabledViaFunc(descriptor))
>  
>          if descriptor.concrete:
>              if descriptor.proxy:
> +                if descriptor.interface.totalMembersInSlots != 0 and False:

Er, I think you should drop |and False|.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> FIXED_SLOTS_MAX seems to be 31?

Hmm.  bhackett had told me 16.  And that there was no sane way to assert this stuff.

Jason, is there a way DOM code can statically assert how many fixed slots we can have?

> Er, I think you should drop |and False|.

Er, yes.  I was testing something and forgot to revert that part.  Good catch!

Comment 51

[Boris Zbarsky [:bzbarsky]]

Let's try actually needinfoing Jason for the bit in comment 50.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Talked to Jason on IRC.  We'll add a friend API constant for max reserved slots.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Add friend API exposing the max number of fixed slots, and some static asserts that we're not going past it.

Comment 54

[Jason Orendorff [:jorendorff]]

Comment on attachment 8337890 [details] [diff] [review]
part 6.  Add friend API exposing the max number of fixed slots, and some static asserts that we're not going past it.

Review of attachment 8337890 [details] [diff] [review]:
-----------------------------------------------------------------

You wrote:
 <bz>	 jorendorff: Shape::FIXED_SLOTS_MAX is the biggest count of fixed slots a Shape can store
 <bz>	 jorendorff: JSObject::MAX_FIXED_SLOTS is the biggest number of fixed slots our GC size classes will give an object

Please put those in as one-line comments on the definitions of these constants in vm/Shape.h and jsobj.h respectively.

::: dom/bindings/Codegen.py
@@ +2479,5 @@
>                  "}\n"
>                  "js::SetReservedSlot(aWrapper, %d, args.rval());" %
> +                (m.slotIndex + INSTANCE_RESERVED_SLOTS,
> +                 self.descriptor.interface.identifier.name, m.identifier.name,
> +                 m.identifier.name, m.slotIndex + INSTANCE_RESERVED_SLOTS))

Micro-nit: I would common up the index computation and give it a local variable.

::: js/src/jsfriendapi.h
@@ +408,5 @@
>              return fixedSlots()[slot];
>          return slots[slot - nfixed];
>      }
> +
> +    static const uint32_t MAX_FIXED_SLOTS = 16;

Comment the actual contract here: reserved slots with index <= this number are guaranteed to be fixed. (Right?)

::: js/src/vm/Shape.h
@@ +1312,5 @@
>          JS_STATIC_ASSERT(offsetof(Shape, slotInfo) == offsetof(js::shadow::Shape, slotInfo));
>          JS_STATIC_ASSERT(FIXED_SLOTS_SHIFT == js::shadow::Shape::FIXED_SLOTS_SHIFT);
> +        static_assert(js::shadow::Object::MAX_FIXED_SLOTS <= FIXED_SLOTS_MAX,
> +                      "Our public maximum number of fixed slots should fit "
> +                      "in our fixed slot count storage");

Comment golf! I love this game. My best:

    "verify numFixedSlots() bitfield is big enough"

Yours is fine, but the word "public" is a red herring.

Comment 55

[Boris Zbarsky [:bzbarsky]]

> Micro-nit: I would common up the index computation and give it a local variable.

Pain inside array comprehensions.  I went with:

        def slotIndex(member):
            return member.slotIndex + INSTANCE_RESERVED_SLOTS

outside the comprehension and slotIndex(m) inside the comprehension.

Fixed up the rest, though it's reserved slots with index < MAX_FIXED_SLOTS, not <=.  Thank you for the review (and the excellent comment suggestions).

Comment 56

[Boris Zbarsky [:bzbarsky]]

Per irc discussion, I tweaked part 1 to disallow StoreInSlot on attributes with a throwing getter, since that combination makes no sense.

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ad99f6a10a
https://hg.mozilla.org/integration/mozilla-inbound/rev/733f98e1f5d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/aefcfaae2534
https://hg.mozilla.org/integration/mozilla-inbound/rev/96db6fab0b50
https://hg.mozilla.org/integration/mozilla-inbound/rev/717a05a70859
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4592ec867ad

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70ad99f6a10a
https://hg.mozilla.org/mozilla-central/rev/733f98e1f5d0
https://hg.mozilla.org/mozilla-central/rev/aefcfaae2534
https://hg.mozilla.org/mozilla-central/rev/96db6fab0b50
https://hg.mozilla.org/mozilla-central/rev/717a05a70859
https://hg.mozilla.org/mozilla-central/rev/b4592ec867ad

Comment 58

[Boris Zbarsky [:bzbarsky]]

Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#StoreInSlot