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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Posted patch PatchSplinter Review
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.)
Attachment #8962550 - Flags: review?(jdemooij)
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? :/
Attachment #8962550 - Flags: review?(jdemooij) → review+
Priority: -- → P3
Pushed by jwalden@mit.edu:
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
(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.  
...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.
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 #
Flags: needinfo?(jwalden+bmo)
Pushed by jwalden@mit.edu:
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
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.
Attachment #8962990 - Flags: review?(jdemooij)
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
Attachment #8962992 - Flags: review?(sphink)
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.
Attachment #8962990 - Flags: review?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/64eaee27492a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Attachment #8963354 - Flags: review?(jdemooij)
Attachment #8962990 - Attachment is obsolete: true
If I'm touching all these lines, might as well go all the way and apply code style too.
Attachment #8963388 - Flags: review?(sphink)
Attachment #8962992 - Attachment is obsolete: true
Attachment #8962992 - Flags: review?(sphink)
Flags: needinfo?(jwalden+bmo)
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?
Attachment #8963388 - Flags: review?(sphink) → review+
Comment on attachment 8963354 [details] [diff] [review]
Now using toNunboxPayload

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

Thanks!
Attachment #8963354 - Flags: review?(jdemooij) → review+
(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.
Pushed by jwalden@mit.edu:
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
Depends on: 1451248
You need to log in before you can comment on or make changes to this bug.