1449051 - Consolidate the definition of JS::Value::layout to be more readable/understandable and to common up the parts that are common across endianness/word size

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jeff Walden [:Waldo]]

Value::layout's representation is unreadably sprawling, and it's nigh impossible to tell what parts are common across what axes.  Let's define it with targeted #ifs around the unshared/half-shared arms.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

1 files changed, 38 insertions(+), 87 deletions(-)

Motivation for this (aside from the obvious) was that I was looking at the gdb jsval.py integration, and it has a huge comment that tries to regurgitate -- in a somewhat obsolete manner -- all of these internal guts' layout.  With this change a lot of that can be turned into a "see Value.h" reference, IMO.  (In a separate bug.)

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8962550 [details] [diff] [review]
Patch

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

Nice. r=me with comments addressed.

::: js/public/Value.h
@@ +837,5 @@
>          uint64_t asBits;
> +        double asDouble;
> +        void* asPtr;
> +#if defined(JS_PUNBOX64)
> +        size_t asWord;

According to searchfox, asWord is only used in Value::payloadWord and that is unused. Same for asUIntPtr/payloadUIntPtr, so if we remove them you also don't need this #if :)

@@ +878,5 @@
>                  JSWhyMagic     why;
>                  size_t         word;
>                  uintptr_t      uintptr;
>              } payload;
>              JSValueTag tag;

I think here we need to swap tag/payload if we're on big endian? :/

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a446d356da
Consolidate the definition of JS::Value::layout to be more readable/understandable and to common up the parts that are common across endianness/word size.  r=jandem

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> According to searchfox, asWord is only used in Value::payloadWord and that
> is unused.

Righto.

> Same for asUIntPtr/payloadUIntPtr, so if we remove them you also
> don't need this #if :)

Wrongo.  The latter is used in some JitFrames.cpp JS_NUNBOX32-only code, that presumably isn't compiled by searchfox code-coverage, I guess.  

Comment 5

[Jeff Walden [:Waldo]]

...U+1F4AF HUNDRED POINTS SYMBOL

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/js/src/jit/JitFrames.cpp#934

We probably still can remove it, just it'll mean adding some more union fields or something.  I'm deferring this to a second patch to get the bulk of the simplification landed quicker.

> I think here we need to swap tag/payload if we're on big endian? :/

Bah, I thought I'd double-checked and this was actually how it had been.  Fixt.

Comment 6

[Stefan Hindli [:stefan_hindli]]

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9a145bccd8870c361551b11fa20ffda97c82bc31&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=dabcac07c6835679aaeb585b3d991f5a805a3ffb&selectedJob=170669429
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7fabe2630df68ea746ede62b951a27ca782b71fb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=170669429&repo=mozilla-inbound&lineNumber=1272

task 2018-03-27T22:04:11.783Z] 22:04:11     INFO -  TypeError: coercing to Unicode: need string or buffer, NoneType found
[task 2018-03-27T22:04:14.338Z] 22:04:14     INFO -  --DOCSHELL 0x7f1df2dbe800 == 5 [pid = 804] [id = {13731511-00ec-4136-86d1-f33d31cd325d}]
[task 2018-03-27T22:04:19.077Z] 22:04:19     INFO -  --DOMWINDOW == 15 (0x7f1dfc0ab800) [pid = 804] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:19.079Z] 22:04:19     INFO -  --DOMWINDOW == 14 (0x7f1de965f400) [pid = 804] [serial = 12] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:19.079Z] 22:04:19     INFO -  --DOMWINDOW == 13 (0x7f1de965f000) [pid = 804] [serial = 11] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:19.080Z] 22:04:19     INFO -  --DOMWINDOW == 12 (0x7f1deee92800) [pid = 804] [serial = 15] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:22.762Z] 22:04:22     INFO -  --DOMWINDOW == 11 (0x7f1df2d0d800) [pid = 804] [serial = 7] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:22.823Z] 22:04:22     INFO -  --DOMWINDOW == 2 (0x7fa81b70e800) [pid = 846] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:28.826Z] 22:04:28     INFO -  --DOMWINDOW == 10 (0x7f1df1ccc000) [pid = 804] [serial = 8] [outer = (nil)] [url = about:blank]
[task 2018-03-27T22:04:29.025Z] 22:04:29     INFO -  [Parent 804, StreamTrans #33] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/modules/libjar/nsJARChannel.cpp, line 428
[task 2018-03-27T22:21:09.060Z] 22:21:09     INFO - Automation Error: mozprocess timed out after 1000 seconds running ['/builds/worker/workspace/build/venv/bin/python', '-u', '/builds/worker/workspace/build/tests/marionette/harness/marionette_harness/runtests.py', '--headless', '--gecko-log=-', '--log-raw=-', '-vv', '--log-raw=/builds/worker/workspace/build/blobber_upload_dir/marionette_raw.log', '--log-errorsummary=/builds/worker/workspace/build/blobber_upload_dir/marionette_errorsummary.log', '--log-html=/builds/worker/workspace/build/blobber_upload_dir/report.html', '--binary=/builds/worker/workspace/build/application/firefox/firefox', '--address=localhost:2828', '--symbols-path=/builds/worker/workspace/build/symbols', '/builds/worker/workspace/build/tests/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit-tests.ini']
[task 2018-03-27T22:21:09.071Z] 22:21:09    ERROR - timed out after 1000 seconds of no output
[task 2018-03-27T22:21:09.072Z] 22:21:09    ERROR - Return code: -15
[task 2018-03-27T22:21:09.072Z] 22:21:09    ERROR - No checks run.
[task 2018-03-27T22:21:09.073Z] 22:21:09    ERROR - No suite end message was emitted by this harness.
[task 2018-03-27T22:21:09.074Z] 22:21:09     INFO - TinderboxPrint: marionette<br/><em class="testfail">T-FAIL</em>
[task 2018-03-27T22:21:09.075Z] 22:21:09     INFO - gecko.log not found
[task 2018-03-27T22:21:09.075Z] 22:21:09     INFO - TinderboxPrint: marionette<br/>0/0/0
[task 2018-03-27T22:21:09.077Z] 22:21:09     INFO - Marionette exited with return code -15: FAILURE
[task 2018-03-27T22:21:09.077Z] 22:21:09    ERROR - # TBPL FAILURE #

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64eaee27492a
Consolidate the definition of JS::Value::layout to be more readable/understandable and to common up the parts that are common across endianness/word size.  r=jandem

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Further minifications to JS::Value's internal structure

asUIntPtr *is* used with JS_NUNBOX32, so a little care is required with it.  We could make payloadUintPtr nunbox-only, but I'm not a big fan of functions that only exist depending on arch when they can be defined everywhere, even if not used everywhere.  So, this approach.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Convert JS::Value from being a class containing a JS::Value::layout union, to *itself* being a union containing JS::Value::layout's members

No good reason not to go all the way here now, amirite?  Lots of change here, but the overwhelming portion is just s/data\.//g.  Try runneth.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e6882ed99b9edad014fa7f41287a84b6969220e

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8962990 [details] [diff] [review]
Further minifications to JS::Value's internal structure

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

::: js/public/Value.h
@@ +812,5 @@
>          return toTag() == JSVAL_TAG_PRIVATE_GCTHING;
>      }
>  
>      const uintptr_t* payloadUIntPtr() const {
> +        return &data.ptr.asUIntPtr;

I'd prefer for JitFrames to do either:

  uintptr_t rawPayload = uintptr_t(v.toGCThing());

or

  uintptr_t rawPayload = v.toNunboxPayload();

I don't like payloadUIntPtr because it returns a pointer into our Value - it's a const uintptr at least, but still - and we don't need that in TraceIonJSFrame.

@@ +827,5 @@
>          uint64_t asBits;
>          double asDouble;
> +        struct {
> +#if defined(JS_NUNBOX32) && MOZ_BIG_ENDIAN
> +            JSValueTag : 8;

JSValueTag needs 32 bits, unlike JSValueType.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/64eaee27492a

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Now using toNunboxPayload

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Move JS::Value::layout's members into JS::Value, make JS::Value a union, and apply correct code style to the union fields

If I'm touching all these lines, might as well go all the way and apply code style too.

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8963388 [details] [diff] [review]
Move JS::Value::layout's members into JS::Value, make JS::Value a union, and apply correct code style to the union fields

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

I think just about all of my review comments are pre-existing, so can be ignored for this patch.

::: js/public/Value.h
@@ +344,5 @@
> +    struct {
> +#if defined(JS_PUNBOX64)
> +#  if MOZ_BIG_ENDIAN
> +        uint32_t : 32;
> +#  endif // MOZ_BIG_ENDIAN

Given that I stumbled over this ("missing field name!"), perhaps instead of removing it entirely, say

    uint32_t : 32; // padding

?

@@ +350,5 @@
> +            int32_t i32_;
> +            uint32_t u32_;
> +            JSWhyMagic why_;
> +        } payload_;
> +#else

seems like the rest of this file uses

    #elif defined(JS_NUNBOX32)

here instead of #else.

@@ +374,5 @@
> +
> +  public:
> +    /*
> +     * N.B. the default constructor leaves Value unitialized. Adding a default
> +     * constructor prevents Value from being stored in a union.

*uninitialized

@@ +387,5 @@
> +    void staticAssertions() {
> +        JS_STATIC_ASSERT(sizeof(JSValueType) == 1);
> +        JS_STATIC_ASSERT(sizeof(JSValueTag) == 4);
> +        JS_STATIC_ASSERT(sizeof(JSWhyMagic) <= 4);
> +        JS_STATIC_ASSERT(sizeof(Value) == 8);

I thought we switched to static_assert?

@@ +561,5 @@
>  
>      void swap(Value& rhs) {
> +        uint64_t tmp = rhs.asBits_;
> +        rhs.asBits_ = asBits_;
> +        asBits_ = tmp;

Perhaps not for this patch, but it seems like this would read better as mozilla::Swap(asBits_, rhs.asBits_);

::: js/src/gdb/mozilla/prettyprinters.py
@@ +226,5 @@
>  template_regexp = re.compile("([\w_:]+)<")
>  
> +def is_struct_or_union(t):
> +    return t.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION)
> +

I guess this does is_class_or_struct_or_union; presumably classes get TYPE_CODE_STRUCT?

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8963354 [details] [diff] [review]
Now using toNunboxPayload

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

Thanks!

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #14)
>     uint32_t : 32; // padding

WFM.

> seems like the rest of this file uses
> 
>     #elif defined(JS_NUNBOX32)

Done, but I don't like the existing names.  We have a 32-bit structuring, and we have a 64-bit structuring.  "nun" and "pun" are meaningless, and we only use them as alternation, so really there should be one name where true is one meaning and false is the other.  Left for later, tho.

> I thought we switched to static_assert?

Okay, changed.  We have switched, but there are a lot of existing uses of the old one that need fixing.  And because the new style requires a reason-string, I've been unwilling to do a search-and-replace versus actually adding sensible reason-strings in a reasoned conversion.

> I guess this does is_class_or_struct_or_union; presumably classes get
> TYPE_CODE_STRUCT?

I assume so.  All I know is when I had the rest of this patch *without* this bit of change, JS::Value would *not* prettyprint, and this sort of thing seemed the likeliest explanation, and indeed there was a failure to consider unions for prettyprinting when I looked.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92d1872f4a23
Further minifications to JS::Value's internal structure.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05e09c6fde3
Move JS::Value::layout's members into JS::Value, make JS::Value a union, and apply correct code style to the union fields.  r=sfink

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/92d1872f4a23
https://hg.mozilla.org/mozilla-central/rev/a05e09c6fde3

Comment 19

[Steve Fink [:sfink] [:s:]]

Note that the rust job failed on try as well: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-searchStr=rust&selectedJob=171137708