rewrite #if magic for defining jsval_layout to use template magic instead

NEW
Unassigned

Status

()

Core
JavaScript Engine
4 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
JS_BITS_PER_WORD is bad news; let's eliminate its use in defining jsval_layout.
(Reporter)

Comment 1

4 years ago
Created attachment 815090 [details] [diff] [review]
rewrite #if magic for defining jsval_layout to use template magic instead

This patch seems to compile and pass tests on Linux.  Almost thought about templating
payloads, but it didn't seem worth the trouble.
Attachment #815090 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

4 years ago
Blocks: 781171
(Reporter)

Comment 2

4 years ago
(In reply to Nathan Froyd (:froydnj) from comment #1)
> Created attachment 815090 [details] [diff] [review]
> rewrite #if magic for defining jsval_layout to use template magic instead
> 
> This patch seems to compile and pass tests on Linux.  Almost thought about
> templating
> payloads, but it didn't seem worth the trouble.

So, I'm not totally sure that this is the right approach.  One wrinkle is that layout<8> needs to at least compile properly on 32-bit machines, as well as 64-bit ones, since it's now defined unconditionally.  (This issue caused a compilation failure on Android and 32-bit Linux.)  And assuming that more things get converted to use template specialization in this file, they also must compile in both ways.  And that's liable to get complicated rather quickly...I think.

An alternative is to keep all the ifdeffery, but remove the JS_BITS_PER_WORD usages and replace them with something like:

#if defined(__LP64__) || defined(_WIN64)
#define JS_VALUE_64BIT
#else
#define JS_VALUE_32BIT
#endif

#if defined(JS_VALUE_64BIT)
JS_STATIC_ASSERT(sizeof(void*) == 8)
#elif defined(JS_VALUE_32BIT)
JS_STATIC_ASSERT(sizeof(void*) == 4)
#endif

...
#undef JS_VALUE_64BIT
#undef JS_VALUE_32BIT

and then convert all the conditionals on JS_BITS_PER_WORD to use the appropriate JS_VALUE_*BIT macro.  That doesn't make Value.h any cleaner, but it does solve the immediate problem of not using JS_BITS_PER_WORD.  WDYT?
Flags: needinfo?(jwalden+bmo)
Comment on attachment 815090 [details] [diff] [review]
rewrite #if magic for defining jsval_layout to use template magic instead

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

What you call "wrinkles" and "complicated", I would call "features".  :-)  Seems to me that knowing immediately when this stuff goes awry is far preferable to not knowing about it at all until that one Solaris dude files a bug months later.

::: js/public/Value.h
@@ +239,5 @@
>      JS_ION_BAILOUT,              /* status code to signal EnterIon will OSR into Interpret */
>      JS_GENERIC_MAGIC             /* for local use */
>  } JSWhyMagic;
>  
> +namespace js {

I'd put this in JS::detail, myself, and leave js::detail for detaily bits of purely-internal stuff, not public stuff like JS::Value.

@@ +242,5 @@
>  
> +namespace js {
> +namespace detail {
> +
> +template<size_t wordsize>

WordSize

@@ +245,5 @@
> +
> +template<size_t wordsize>
> +union layout;
> +
> +union payload32bit {

Class names should be InterCaps, please.  (jsval_layout is kind of a mistake right now.)

@@ +248,5 @@
> +
> +union payload32bit {
> +    int32_t        i32;
> +    uint32_t       u32;
> +    uint32_t       boo;     // Don't use |bool| -- it must be four bytes.

I sort of wonder if this comment's accurate any more, post s/JSBool/bool/g (well, it probably is -- the question is whether it *needs* to be this way, or whether we could change JITs to permit a bool here).  Nothing for you to do about this, certainly, just observing.

@@ +263,3 @@
>      uint64_t asBits;
>      struct {
> +#if MOZ_LITTLE_ENDIAN

Hmm, we need to do the NativeEndian selection trick I mentioned we should have when Endian landed, to get rid of this preprocessor-selection.  I guess later.

@@ +295,5 @@
> +        union payload64bit payload;
> +        uint32_t padding;
> +#else
> +        uint32_t padding;
> +        union payload64bit payload;

I'd get rid of the unnecessary |union| prefixing in this situation, both arms here and above.

@@ +302,5 @@
>      double asDouble;
>      void *asPtr;
>      size_t asWord;
>      uintptr_t asUIntPtr;
> +} JSVAL_ALIGNMENT;

I'm a little surprised this goes here, and not after layout<8>.  If it compiles after that, I'd prefer that location, and same for layout<4>.
Attachment #815090 - Flags: review?(jwalden+bmo) → review+
Flags: needinfo?(jwalden+bmo)
> > +union payload32bit {
> > +    int32_t        i32;
> > +    uint32_t       u32;
> > +    uint32_t       boo;     // Don't use |bool| -- it must be four bytes.
> 
> I sort of wonder if this comment's accurate any more, post s/JSBool/bool/g
> (well, it probably is -- the question is whether it *needs* to be this way,
> or whether we could change JITs to permit a bool here).

I added this comment during the JSBool-removal.  IIRC keeping this as a 32-bit value was crucial, even without the JITs.  See bug 898914 comment 26.
The explanation I got on IRC is that x86 and friends aren't CISC-y enough for bool+uint32_t to be fast.  So things should be this way, but the comment should be updated.  (But not in this bug, or at least not as part of holding up the patch here.)
You need to log in before you can comment on or make changes to this bug.