Closed
Bug 587914
Opened 14 years ago
Closed 14 years ago
js::Valueify in static constructors causes runtime initialization
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: luke)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
22.90 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•14 years ago
|
||
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".
Comment 2•14 years ago
|
||
(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.
Assignee | ||
Comment 3•14 years ago
|
||
(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.
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
(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 :)
Comment 6•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
Igor: any plans to do this?
Assignee | ||
Comment 8•14 years ago
|
||
Comment 9•14 years ago
|
||
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+
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/eb83de9d08da
Whiteboard: fixed-in-tracemonkey
Comment 11•14 years ago
|
||
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.
Description
•