Closed Bug 587914 Opened 14 years ago Closed 14 years ago

js::Valueify in static constructors causes runtime initialization

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

bug 571789 changed a bunch of JSClass's to js::Class's and changed the static class-initialization code to use js::Valueify to convert jsval-typed functions to js::Value-typed functions.  (http://hg.mozilla.org/tracemonkey/rev/77eb248fa854) This causes runtime-initialization code to be emitted which will hurt startup perf: each class instance initialized this way will be a separate pre-main function call.

I am not a fan of replacing these js::Valueify's with casts, since that circumvents static type checking.  Perhaps another struct, containing jsval types, could be whipped up to mirror the extended contents of js::Class?
I forgot to mention, the mirror-structure idea would avoid a lot of gnarly js::Valueify/js::Jsvalify in non-js/src code.  The general idea is to have code use either one or the other so casts are only necessary "at the boundaries".
(In reply to comment #0)
> I am not a fan of replacing these js::Valueify's with casts, since that
> circumvents static type checking.

One can use macros to enable type-checking only in debug builds. Alternatively xpconnect can start to use js::Value signatures in its methods. 

(In reply to comment #1)
> I forgot to mention, the mirror-structure idea would avoid a lot of gnarly
> js::Valueify/js::Jsvalify in non-js/src code.

The need to use Valueify comes from the simple fact that our public API are not powerful enough to address XPConnect needs. Another mirror-structure would just try to hide this fact besides adding ugliness IMO.
(In reply to comment #2)
> (In reply to comment #0)
> > I am not a fan of replacing these js::Valueify's with casts, since that
> > circumvents static type checking.
> 
> One can use macros to enable type-checking only in debug builds.

js::Valueify can't be macroized as it uses overload resolution to pick the codomain, but maybe you were thinking of adding a new JS_VALUEIFY_NATIVE?

> Alternatively
> xpconnect can start to use js::Value signatures in its methods.

That would be a sizable change and, as XPConnect had only a few uses of js::Valueify/js::Jsvalify (before your patch), there doesn't seem to be much of a benefit.

> (In reply to comment #1)
> > I forgot to mention, the mirror-structure idea would avoid a lot of gnarly
> > js::Valueify/js::Jsvalify in non-js/src code.
> 
> The need to use Valueify comes from the simple fact that our public API are not
> powerful enough to address XPConnect needs. Another mirror-structure would just
> try to hide this fact besides adding ugliness IMO.

The need to Valueify comes from the public API using different types than the private API.  The need to use different types comes from not wanting to rewrite the world with a new jsval and being able to have a private value API that we can tweak for internal purposes.  The mirror structure doesn't hide some problem that would go away by making the public API more "powerful": your patch added functionality to js::Class that apparently is not a private JS engine detail since it is used outside js/src (viz. js/ipc).  Now, maybe js/ipc is another xpconnect and we pretend its private, but if not -- if we expect perhaps other users of the extended js::Class functionality -- then I believe we should mirror this extended functionality in the public jsval-using API.
(In reply to comment #3)
> js::Valueify can't be macroized as it uses overload resolution to pick the
> codomain, but maybe you were thinking of adding a new JS_VALUEIFY_NATIVE?

I was thinking about adding a set of macros sufficient to cover XPConnect needs.

> detail since it is used outside js/src (viz. js/ipc).  Now, maybe js/ipc is
> another xpconnect and we pretend its private,

It is private one AFAIK. As with xpconnect it uses internals for power and efficiency and with knowledge that it will be updates if internals changes. 


> but if not -- if we expect
> perhaps other users of the extended js::Class functionality -- then I believe
> we should mirror this extended functionality in the public jsval-using API.

If we believe that the extended functionality is useful for other embeddings we should just open the API and move the corresponding fields into the public part of JSClass.
(In reply to comment #4)
> > but if not -- if we expect
> > perhaps other users of the extended js::Class functionality -- then I believe
> > we should mirror this extended functionality in the public jsval-using API.
> 
> If we believe that the extended functionality is useful for other embeddings we
> should just open the API and move the corresponding fields into the public part
> of JSClass.

So we are in agreement :)
(In reply to comment #5)
> So we are in agreement :)

My preference would be to make 2-3 hooks like equality or some form of typeof public and cover the rest with macro casts.
Igor: any plans to do this?
Attached patch patchSplinter Review
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #480194 - Flags: review?(igor)
Comment on attachment 480194 [details] [diff] [review]
patch

>diff --git a/js/src/jsvalue.h b/js/src/jsvalue.h
>+# define JS_VALUEIFY(type, v) ((type)v)
>+# define JS_JSVALIFY(type, v) ((type)v)

Nit: add () arround v

>+
> # define JS_VALUEIFY_NATIVE(n) ((js::Native)n)
> # define JS_JSVALIFY_NATIVE(n) ((JSNative)n)

Pre-existing nit: add () arround n
Attachment #480194 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/eb83de9d08da
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/eb83de9d08da
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.