Boris Zbarsky [:bzbarsky] Assignee
	Description • 9 years ago

I want this for bug 928336 so I can quickly copy over properties from a template-like object to my real object.

	Brian Hackett [Laid off!]
	Comment 2 • 9 years ago

Comment on attachment 8566962 [details] [diff] [review]
Add JS friend API to quickly copy properties from one object to another  if the objects are similar enough

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

::: js/src/jsobj.cpp
@@ +1897,5 @@
> +                                               HandleNativeObject src)
> +{
> +    assertSameCompartment(cx, src, dst);
> +    MOZ_ASSERT(src->getClass() == dst->getClass());
> +    MOZ_ASSERT(src->getParent() == dst->getParent());

I think you should also MOZ_ASSERT(src->group() == dst->group()).  Otherwise the element/property updates below could invalidate type information for dst if it has a different group from src.

You also might want to assert dst->getObjectFlags() == 0.  The only other bit which this method doesn't handle is the object metadata, which can be installed by a devtool or something and really ought to be preserved by this function.  It might be best though if you can disable this fast copying path in the caller when the object metadata callback is in use, though, as handling it here could be kind of annoying (e.g. the src itself could have metadata).

@@ +1899,5 @@
> +    assertSameCompartment(cx, src, dst);
> +    MOZ_ASSERT(src->getClass() == dst->getClass());
> +    MOZ_ASSERT(src->getParent() == dst->getParent());
> +
> +    if (!dst->ensureElements(cx, src->getDenseCapacity()))

This can use src->getDenseInitializedLength()

@@ +1925,5 @@
> +    } else if (src->is<ArrayObject>()) {
> +        ObjectGroup::fixArrayGroup(cx, &dst->as<ArrayObject>());
> +    } else if (src->is<PlainObject>()) {
> +        ObjectGroup::fixPlainObjectGroup(cx, &dst->as<PlainObject>());
> +    }

I don't think you want any of this.  The main question here is whether it is possible for src or dst to be a singleton.  External to the API the only way to create singleton objects is with JS_NewObjectWithUniqueType (which needs renaming fwiw), so if none of the objects used with that API can flow here then you don't need to handle it.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 9 years ago

> I think you should also MOZ_ASSERT(src->group() == dst->group())

Will do.

> You also might want to assert dst->getObjectFlags() == 0.

Again, will do.

> It might be best though if you can disable this fast copying path in the caller when the
> object metadata callback is in use, though

Hmm.  So my particular use case actually somewhat relies on the copy function not invoking the addproperty hook on dst even though such a hook exists.  I could try to rejigger things so that there is no such dependency and fall back on JS_CopyPropertiesFrom if the object metadata callback is in use, I guess...

That said, the objects that are "src" here are very much internal implementation details that are not exposed to anyone, so I'd really hope devtools don't mess with them.  :(

If I do need to handle this, how do I detect from outside SpiderMonkey whether the object metadata callback is in use?

> This can use src->getDenseInitializedLength()

Will do.

> I don't think you want any of this. 

Excellent, thanks!  For my use case, src never has unique type.  I'll add an assert that !src->isSingleton().

That said, I think src and dst can in fact be PlainObject in my case.  Do I not need to fixPlainObjectGroup because of the assert that they have the same group()?

	Brian Hackett [Laid off!]
	Comment 4 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #3)
> Hmm.  So my particular use case actually somewhat relies on the copy
> function not invoking the addproperty hook on dst even though such a hook
> exists.  I could try to rejigger things so that there is no such dependency
> and fall back on JS_CopyPropertiesFrom if the object metadata callback is in
> use, I guess...
> 
> That said, the objects that are "src" here are very much internal
> implementation details that are not exposed to anyone, so I'd really hope
> devtools don't mess with them.  :(
> 
> If I do need to handle this, how do I detect from outside SpiderMonkey
> whether the object metadata callback is in use?

Well, maybe the best thing here is:

1) Avoid invoking the metadata callback when invoking the source objects.  This can be done by using an API that wraps the construction of the source objects in an AutoEnterAnalysis.

2) Make sure the API you add here preserves metadata for the dst objects, by loading it before setting the last property, then setting it (if non-NULL) after copying the shape from source, using setObjectMetadata().

That said though, we don't seem to be doing a very good job with this sort of template object stuff and the metadata callback within the JS engine; I don't see anything that prevents metadata from being attached to objects inside scripts during parsing, for example (this used to not be possible but I removed it in bug 950118 for some reason that escapes me).  The worst thing that can really happen in this case though is that we'd end up with some objects that have metadata from the template object rather than their original metadata.  So it might be better to just ignore this issue for now until we do a more comprehensive fix for this issue within the engine.

> That said, I think src and dst can in fact be PlainObject in my case.  Do I
> not need to fixPlainObjectGroup because of the assert that they have the
> same group()?

fixPlainObjectGroup() is for a very specific case, when we have a plain object from JSON or from a script with constant members, and want to change its group from the default unknown-properties one to a group specialized for the combination/order of properties the object has.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 9 years ago

The two-point plan above seems reasonable.  I'll add a JS_NewObjectWithNoMetadata API (just like JS_NewObjectWithGivenProto, but with the AutoEnterAnalysis), add an assert that src has no metadata, and then preserve the dst metadata.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 9 years ago

OK, so there actually is a problem here.

The src->group() == dst->group() assert fails.  If nothing else, because src and dst have different protos, and apparently group() includes proto.

I guess group() is what used to be called type()?

In any case, in my case the prototypes are definitely not the same.

Under what conditions is that OK here?  Note that I'm doing this copying right after creating dst, so it hasn't had a chance to flow out to scripts yet (modulo whatever debugger is doing, I guess).

	Brian Hackett [Laid off!]
	Comment 11 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #10)
> OK, so there actually is a problem here.
> 
> The src->group() == dst->group() assert fails.  If nothing else, because src
> and dst have different protos, and apparently group() includes proto.
> 
> I guess group() is what used to be called type()?
> 
> In any case, in my case the prototypes are definitely not the same.
> 
> Under what conditions is that OK here?  Note that I'm doing this copying
> right after creating dst, so it hasn't had a chance to flow out to scripts
> yet (modulo whatever debugger is doing, I guess).

It's OK if the groups don't match, as long as after the operation dst doesn't have any properties which TI will try to account for, i.e. dense elements or plain data properties.  I see the method is only filling in the object up to JSCLASS_RESERVED_SLOTS; are any of these reserved slots accessible via a data property in src's shape?  If not, there's nothing you need to do, but if so, the type information for dst needs to reflect the possible values being given to the property, and the best/most efficient way to do that depends on what's store in these properties.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 9 years ago

> are any of these reserved slots accessible via a data property in src's shape? 

In my specific case, no.

What I have are some reserved slots that are not exposed via shapes in any way, some accessor properties, and some function-valued data properties.  I'm just trying to copy the accessors and function-valued data properties, really....  The dense elements bits in the patch were just for "completeness"; I don't need them.

So it sounds like I don't need to assert group() equality, but what's a reasonable assert instead?

	Brian Hackett [Laid off!]
	Comment 13 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #12)
> > are any of these reserved slots accessible via a data property in src's shape? 
> 
> In my specific case, no.
> 
> What I have are some reserved slots that are not exposed via shapes in any
> way, some accessor properties, and some function-valued data properties. 
> I'm just trying to copy the accessors and function-valued data properties,
> really....  The dense elements bits in the patch were just for
> "completeness"; I don't need them.
> 
> So it sounds like I don't need to assert group() equality, but what's a
> reasonable assert instead?

I don't think you need any assert regarding the group, then.

However, I missed this in the earlier comment but two objects with the same shape need to have the same prototype.  Simply copying the shape from src to dst is not valid if the two objects have different prototypes.  There's a way around this with the hasUncacheableProto() stuff, but that inhibits optimizations wrt prototype lookups and is probably not what you want.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 9 years ago

Oh, because we assume shape gets changed when the proto chain changes, right...

So here's the thing: I think can guarantee that src and dst will have the same proto for now, but not once we allow subclassing DOM objects: that point src will have the "canonical" proto but dst will have whatever the .prototype of the subclass constructor that got invoked was.

I'm pretty ok with falling back to something slower in that situation, I guess.  Is JS_CopyPropertiesFrom the best we can do at that point, or can we do something in between that and what I have in this patch?

	Brian Hackett [Laid off!]
	Comment 15 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #14)
> Oh, because we assume shape gets changed when the proto chain changes,
> right...
> 
> So here's the thing: I think can guarantee that src and dst will have the
> same proto for now, but not once we allow subclassing DOM objects: that
> point src will have the "canonical" proto but dst will have whatever the
> .prototype of the subclass constructor that got invoked was.
> 
> I'm pretty ok with falling back to something slower in that situation, I
> guess.  Is JS_CopyPropertiesFrom the best we can do at that point, or can we
> do something in between that and what I have in this patch?

It's definitely possible to do much better than what JS_CopyPropertiesFrom is doing.

This is the sort of thing we use weak-reference-holding hashtables for in the JS engine.  In this case you could have a table which is (src object x prototype object) -> (final shape), populating that table as necessary when creating dst objects with various prototypes.  During GC entries with dead references would be swept from the table.  An example of this sort of thing is ObjectGroupCompartment, which has several tables that work this way.  Doing this is definitely a good bit more involved than calling JS_CopyPropertiesFrom, but will be a lot faster.

	Brian Hackett [Laid off!]
	Comment 16 • 9 years ago

Hmm, to elaborate a bit, the table outlined above isn't storing any canonical information: the shapes it is storing can be recovered just by doing the slow JS_CopyPropertiesFrom call, since that will produce canonical shapes according to the contents of the property tree.

This frees up the design options for the table a bit.  You could just wipe it during GC.  You could also have a fixed size cache storing some table entries which you can populate during execution, clobbering entries during collisions and then wiping the cache during GC.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 9 years ago

OK.  So this table would presumably live on the compartment and the table-munging bits would presumably be handled by the JS engine itself, not by the API consumer, right?

I'm going to just assert that the proto is the right thing on entry for now and filed bug 1135963 for the followup to make this work with subclassing.

	Jeff Walden [:Waldo]
	Comment 19 • 9 years ago

Comment on attachment 8567949 [details] [diff] [review]
part 1.  Add JS friend API to allocate an object which is guaranteed to have no attached metadat

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

::: js/src/jsfriendapi.h
@@ +65,5 @@
> +// internal bookkeeping objects that are guaranteed to not have metadata
> +// attached to them.
> +extern JS_FRIEND_API(JSObject *)
> +JS_NewObjectWithoutMetadata(JSContext *cx, const JSClass *clasp, JS::Handle<JSObject*> proto,
> +                            JS::Handle<JSObject*> parent = JS::NullPtr());

Can you remove the parent argument and just have it internally use the global?  It seems like a nuisance we'll regret to allow these to have a weird parent, if the user were so stupid.

	Jeff Walden [:Waldo]
	Comment 20 • 9 years ago

Comment on attachment 8568284 [details] [diff] [review]
part 2.  Add JS friend API to quickly copy properties from one object to another  if the objects are similar enough

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

::: js/src/jsfriendapi.h
@@ +154,5 @@
>  
> +/*
> + * Copy the own properties of src to dst in a fast way.  src and dst
> + * must both be native and must have the same class and the same
> + * parent.  Class reserved slots will NOT be copied.

Hm, maybe not.  :-(

::: js/src/jsobj.cpp
@@ +1930,5 @@
> +    if (!dst->setLastProperty(cx, shape))
> +        return false;
> +    for (size_t i = JSCLASS_RESERVED_SLOTS(src->getClass()); i < span; i++) {
> +        dst->setSlot(i, src->getSlot(i));
> +    }

No braces.

@@ +1933,5 @@
> +        dst->setSlot(i, src->getSlot(i));
> +    }
> +
> +    if (dstMetadata && !js::SetObjectMetadata(cx, dst, dstMetadata))
> +        return false;

if (dstMetadata) {
    if (!js::SetObjectMetadata(cx, dst, dstMetadata))
        return false;
}

so as to make the first condition read more like a guard on an action, please.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 9 years ago

> Can you remove the parent argument and just have it internally use the global? 

Absolutely.  Done.

> No braces.

Done.

> so as to make the first condition read more like a guard on an action, please.

Also done.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0512c8c17930
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a8fca17a4fa

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0512c8c17930
https://hg.mozilla.org/mozilla-central/rev/1a8fca17a4fa