Removing JS_ConstructObject, JS_ConstructObjectWithArguments

[Jeff Walden]

I was looking at various bits of SpiderMonkey code recently and noticed that JS_ConstructObject* is unused outside SpiderMonkey in Gecko. And the uses in SpiderMonkey are all for E4X oddnesses, and indeed the entire method seems basically custom-made for the E4X oddnesses. Would anyone mind overmuch if I removed them? Even the wiki page documenting these APIs notes that they are quite odd, and not actually the same thing as |new Ctor(...)|:

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_ConstructObject

If anyone's actually using these, I'm sure there's a better way to address whatever issue might be occurring.

Jeff

[Peter Wilson]

You're forgetting the SpiderMonkey is used in a number of projects completely
unrelated to Mozilla. We use it in Whitebeam for example to implement
server-side JavaScript.

And yes - we use JS_ConstructObject to create instances of our native classes.

There may be a better way to do this but certainly at the time we first started
using SpiderMonkey there was no other way that I could find (other than
effectively running a wasteful eval on 'new MyClass') to correctly instantiate
an object.

Best Regards
Peter Wilson
--
Architect, Whitebeam Web Application server
http://www.whitebeam.org
--

[Bobby Holley]

On Tue, May 22, 2012 at 9:10 PM, Peter Wilson <pe...@yellowhawk.co.uk>wrote:

> You're forgetting the SpiderMonkey is used in a number of projects
> completely unrelated to Mozilla. We use it in Whitebeam for example to
> implement server-side JavaScript.
>

I don't think he was forgetting. That's why he asked on the list rather
than just ripping it out.

bholley

[Jeff Walden]

On 05/22/2012 12:10 PM, Peter Wilson wrote:
> And yes - we use JS_ConstructObject to create instances of our native classes.
>
> There may be a better way to do this but certainly at the time we first started using SpiderMonkey there was no other way that I could find (other than effectively running a wasteful eval on 'new MyClass') to correctly instantiate an object.

This must have been before JS_New, right? JS_New (passing MyClass for the constructor) is the right way to do |new MyClass(...)| in modern JSAPI, of the sort that'll be in whatever SpiderMonkey release contains the current trunk code. Does JS_New not work for your purposes in any way?

Jeff

[Juan Conejero]

We also use them to instantiate native classes in our application, which
is completely unrelated to Mozilla.

Note that we don't have a 'constructor function object'; just a class
definition and a prototype object, which is the result of a call to
JS_InitClass():

JSObject* global = ...; // our Global object
JSClass* class = { ... };
JSObject* prototype = JS_InitClass( cx, global, 0, class, ctor, ... );
// ...
JSObject* myObj = JS_ConstructObject( cx, class, prototype, 0 );

If there is a way to use JS_New() to achieve exactly the same as the
code above does, then please show it to us, and you can get rid of
JS_ConstructObject*. Otherwise removing these API functions might cause
us a lot of trouble.

Juan Conejero
PixInsight Development Team
http://www.pixinsight.com/

[Jeff Walden]

On 05/24/2012 09:47 AM, Juan Conejero wrote:
> Note that we don't have a 'constructor function object'; just a class definition and a prototype object, which is the result of a call to JS_InitClass():
>
> JSObject* global = ...; // our Global object
> JSClass* class = { ... };
> JSObject* prototype = JS_InitClass( cx, global, 0, class, ctor, ... );
> // ...
> JSObject* myObj = JS_ConstructObject( cx, class, prototype, 0 );

You should be able to get the constructor off the prototype object (just make sure to do it before the prototype's exposed to anything that might remove that property):

Value v;
JS_GetProperty(cx, prototype, "constructor", &v);
Value args[2] = { Int32Value(17), NullValue() };
JS_New(cx, &v.toObject(), 2, args);

Also, as long as you don't have JSCLASS_IS_ANONYMOUS set, you should be able to get the constructor off the global object, because JS_InitClass(cx, global, ....) will put the constructor function in global[class->name]. Modulo the caveat, both ways should be identical if you're acting early enough.

Jeff

[Juan Conejero]

Hi Jeff,

I can confirm that this works fine with 1.8.5 (error checking removed
for simplicity):

jsval val;
JS_GetProperty( cx, prototype, "constructor", &val );
JSObject* ctor;
JS_ValueToObject( cx, val, &ctor );
JSObject* myObj = JS_New( cx, ctor, 0, 0 );

Thanks for pointing out this workaround. However, doesn't
JS_ConstructObject() seem an easier and more convenient way (from the
embedder's POV) to instantiate native classes? If there's no compelling
reason to remove them, may I suggest that you preserve these API
functions in future versions?

Juan Conejero
PixInsight Development Team
http://www.pixinsight.com/

On 05/24/2012 07:12 PM, Jeff Walden wrote:
> On 05/24/2012 09:47 AM, Juan Conejero wrote:
>> Note that we don't have a 'constructor function object'; just a class definition and a prototype object, which is the result of a call to JS_InitClass():
>>
>> JSObject* global = ...; // our Global object
>> JSClass* class = { ... };
>> JSObject* prototype = JS_InitClass( cx, global, 0, class, ctor, ... );
>> // ...
>> JSObject* myObj = JS_ConstructObject( cx, class, prototype, 0 );
>
> You should be able to get the constructor off the prototype object (just make sure to do it before the prototype's exposed to anything that might remove that property):
>
> Value v;
> JS_GetProperty(cx, prototype, "constructor",&v);
> Value args[2] = { Int32Value(17), NullValue() };

> JS_New(cx,&v.toObject(), 2, args);

[Jeff Walden]

On 05/25/2012 10:24 AM, Juan Conejero wrote:
> However, doesn't JS_ConstructObject() seem an easier and more convenient way (from the embedder's POV) to instantiate native classes?

It's more convenient because JS_InitClass returns the new prototype, and doesn't directly return the new constructor. I expect we'll make the constructor more prominent in the class API to address this at some point.

But JS_ConstructObject is also slower than JS_New because it recomputes the constructor function every time it's called. And it can't always compute it correctly. JS_ConstructObject(cx, clasp, parent) can throw or return a nonsensical value if anyone overwrites |global[clasp->name]|. This is an API design flaw that can't be worked around.

But if you wanted to keep using JS_ConstructObject warts and all, jorendorff suggests this (modulo any typos) would work:

JSObject *
JS_ConstructObjectWithArguments(JSContext *cx, JSClass *clasp,
JSObject *parent, unsigned argc, jsval *argv)
{
JSObject *global = JS_GetGlobalForScopeChain(cx);
jsval v;
if (!global || !JS_GetProperty(cx, global, clasp->name, &v))
return NULL;
if (JSVAL_IS_PRIMITIVE(v)) {
JS_ReportError(cx, "cannot construct object: constructor is gone");
return NULL;
}
return JS_New(cx, JSVAL_TO_OBJECT(v), argc, argv);
}

JSObject *
JS_ConstructObject(JSContext *cx, JSClass *clasp, JSObject *parent)
{
return JS_ConstructObjectWithArguments(cx, clasp, parent, 0, NULL);

}

> If there's no compelling reason to remove them, may I suggest that you preserve these API functions in future versions?

The API design flaw bit is the compelling reason not to preserve this. If we provide an API and simply hope people won't use it, we'll end up disappointed, even just because those people didn't know any better.

Jeff

[Jeff Walden]

Thus far it sounds like JS_New and various steps are adequate for people's purposes here if JS_ConstructObject* were removed. So I'm requesting review on the patch to remove the two methods now, and I'll land it shortly after it gets review, probably early next week.

If you have some concern that we haven't yet addressed here, speak now, please!

Jeff

[Peter Wilson]

We're still officially using the release before 1.8.5 (1.7) - when the API was
still stable. We have a 'work in progress' porting to 1.8.5 which is proving
something of a nightmare. Since then the API seems to be a moving target.

I've always favoured backwards compatibility as APIs evolve creating the minimum
barrier to people upgrading. This certainly worked fine for SpiderMonkey for
many years.

JS_New doesn't exist in 1.7.

Isn't the correct approach here to leave JS_ConstructObj in the API for
compatibility and make it a wrapper for the new mechanism?

Best regards
Pete
--

[Dave Mandelin]

On Friday, May 25, 2012 3:38:30 PM UTC-7, Jeff Walden wrote:
> On 05/25/2012 10:24 AM, Juan Conejero wrote:
> > However, doesn't JS_ConstructObject() seem an easier and more convenient way (from the embedder's POV) to instantiate native classes?
>
> It's more convenient because JS_InitClass returns the new prototype, and doesn't directly return the new constructor. I expect we'll make the constructor more prominent in the class API to address this at some point.
>
> But JS_ConstructObject is also slower than JS_New because it recomputes the constructor function every time it's called. And it can't always compute it correctly. JS_ConstructObject(cx, clasp, parent) can throw or return a nonsensical value if anyone overwrites |global[clasp->name]|. This is an API design flaw that can't be worked around.

And to add a bit more from a conversation Jeff and I had about this earlier:

I think we should be biased against providing utility methods in the API that just wrap other APIs, because it bloats the API and makes it harder to change. I don't see a compelling case for JS_ConstructObject given that it can be replicated easily enough with other APIs. Plus it has the perf and semantics problems Jeff noted.

Looking further out, I think that the JSClass concept should be wrapped up and hidden from most parts of the API. Jeff's been doing a lot of work to make internal and external APIs consonant with the spec, which is great for API stability and semantics. JSClass doesn't exist in the spec, which just refers to "host objects". IMO the right way to do them in the API is to provide a narrow API including a function that takes in a description of the host object (e.g., what to do when getting a property, tracing for GC, etc; aka the JSClass) and returns a constructor as a normal jsval (or perhaps object), plus a minimal set of support functions (e.g. JS_GetClass assuming it's truly necessary). That means functions that take JSClass * and aren't truly necessary are on the hit list.

Dave

[Dave Mandelin]

On Thursday, May 31, 2012 10:50:28 AM UTC-7, Peter Wilson wrote:
> On 22/05/2012 22:41, Jeff Walden wrote:
> > On 05/22/2012 12:10 PM, Peter Wilson wrote:
> >> And yes - we use JS_ConstructObject to create instances of our native classes.
> >>
> >> There may be a better way to do this but certainly at the time we first started using SpiderMonkey there was no other way that I could find (other than effectively running a wasteful eval on 'new MyClass') to correctly instantiate an object.
> >
> > This must have been before JS_New, right? JS_New (passing MyClass for the constructor) is the right way to do |new MyClass(...)| in modern JSAPI, of the sort that'll be in whatever SpiderMonkey release contains the current trunk code. Does JS_New not work for your purposes in any way?
> >
> > Jeff
> We're still officially using the release before 1.8.5 (1.7) - when the API was
> still stable. We have a 'work in progress' porting to 1.8.5 which is proving
> something of a nightmare. Since then the API seems to be a moving target.
>
> I've always favoured backwards compatibility as APIs evolve creating the minimum
> barrier to people upgrading. This certainly worked fine for SpiderMonkey for
> many years.

Of course; backward compatibility is the normal thing to do. I don't see how we can maintain compatibility in the current environment, though. We need to change APIs quite a bit to support things like modern GC, compartments, and new jsvals (in the JM days).

In a way, we have been building a new SpiderMonkey out of old SpiderMonkey parts, and the new one needs a new API. The interim "releases" are snapshots of iterations within that overhaul. Creating a new stable API will be a lot of work and only makes sense to do after the engine itself has stabilized.

> JS_New doesn't exist in 1.7.
>
> Isn't the correct approach here to leave JS_ConstructObj in the API for
> compatibility and make it a wrapper for the new mechanism?

What about taking the JS_ConstructObject code Jeff posted and putting it in your project, i.e., moving the function from API to your code?

Dave

[Wes Garland]

On 31 May 2012 13:50, Peter Wilson <pe...@yellowhawk.co.uk> wrote:

> We're still officially using the release before 1.8.5 (1.7) - when the API
> was still stable. We have a 'work in progress' porting to 1.8.5 which is
> proving something of a nightmare. Since then the API seems to be a moving
> target.
>

> ....

> I've always favoured backwards compatibility as APIs evolve creating the
> minimum barrier to people upgrading. This certainly worked fine for
> SpiderMonkey for many years.
>
> JS_New doesn't exist in 1.7.
>
> Isn't the correct approach here to leave JS_ConstructObj in the API for
> compatibility and make it a wrapper for the new mechanism?
>

The approach *I* have been taking is to implement old API from new, as best
as possible, via macros and so on to try and isolate myself and my users
from JSAPI version drift.

Oddly enough, it is this effort that has been the biggest stumbling block
to getting up to 1.8.5. I think we run on most versions between 1.7 and
~1.8.2 (Firefox 3.6), build system issues aside.

That said, there are simply APIs which are not implementable exactly as-is
from old JSAPI to new JSAPI, and new JSAPI is required for the performance
improvements and language features we need. So I have thrown in the towel
and actually encourage Moz to churn the API as fast as possible while it is
in this state of flux, so that we can hopefully eventually arrive at a
stable API and a great engine in the next year or two. We've already got
one of those two conditions. :)

Wes

--
Wesley W. Garland
Director, Product Development
PageMail, Inc.
+1 613 542 2787 x 102