1485347 - Refactor JSScript variable-length data storage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ted Campbell [:tcampbell]]

The JSScript is a GCThing with a fixed storage and so we hang off variable-length arrays on JSScript::data. I'd propose defining PrivateScriptData struct as the sister-type of SharedScriptData and move some of the variable-length encoding goo off the JSSscript and into this new PrivateScriptData.

At the same time, I'd like to replace ScopeArray (and friends) with a u32 offset and length. This will save memory on 64-bit due to pointer size and packing issues. The accessors will instead unpack the offset and return a mozilla::Span.

Another detail is that we currently use a few flags in JSScript that we test in a daisy-chain in order to compute the location of these FooArray structures. In the new proposal, these flags will be widened to a 4-bit index. An index of 0 indicates the array is empty as before, but a non-zero will indicate which span descriptor to use. This greatly simplifies access to the various data arrays while still maintaining the original size optimizations. This flag word can be packed into PrivateScriptData where ScopeArray to make this size-neutral or better (even on 32-bit).

This cleanup will allow simpler movement of script data between JSScript / PrivateScriptData / SharedScriptData.

Comment 1

[Ted Campbell [:tcampbell]]

Also, the consts() array is only used for doubles these days, so we should strength-reduce the GCPtrValue to just JS::Value and remove the tracing of that array. Might need to be a little careful around hazard analysis if we do.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Part 1: Remove nTypeSets argument from JSScript::partiallyInit

MozReview-Commit-ID: I6xBHG8FLlq

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Part 2: Remove CheckScriptDataIntegrity.

This diagnostic did not provide value and should be removed to allow
further cleanup.

MozReview-Commit-ID: 49XJM0G7NAP

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Part 3: Use mozilla::Span for JSScript::data arrays

Replace various custom data-types in JSScript interfaces with
mozilla::Span. This abstracts implementation details and supports
range-based for-loops. Underlying storage is unchanged, but this sets us
up to be able to more easily change it.

MozReview-Commit-ID: FDfIYsAxTA8

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Part 4: Add js::PrivateScriptData type

This will later be used to store variable length data that hangs off of
each JSScript. This primarily is a refactor of existing JSScript code to
put in its own data structure.

- ScopeArray and friends now store offsets instead of pointers. This
  saves memory on 64-bit platforms and simplifies cloning.
- GCPtr constructors are used instead of relying on pod_calloc. This
  fixes C++ object-model violations.
- A packed bitfield is used to locate optional array headers instead of
  previous daisy-chain approach. This also lets js::PrivateScriptData
  understand array layout without coupling to JSScript.

MozReview-Commit-ID: AeSyTSPFtJY

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Part 5: Use js::PrivateScriptData for JSScript

- This makes JSScript::data arrays read-only. Initialization code
 directly uses PrivateScriptData to mutate.

MozReview-Commit-ID: LJFc8QazLfq

Comment 7

[Ted Campbell [:tcampbell]]

Additional patches I haven't posted yet..
 - Part 6: Cleanup js::SharedScriptData to be similar to js::PrivateScriptData. It removes the |uintptr_t data[1]| field.
 - Part 7: Refactor JSScript constructor/partiallyInit/etc to be more explicit about object initialization.
 - Bug ???: Refactor XDRScript and CopyScript to have less C++ violations and use helper methods for PrivateScriptData/SharedScriptData.

It is too late in cycle to land any of this stuff and I still need further testing for perf regressions.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 9003943 [details] [diff] [review]
Part 3: Use mozilla::Span for JSScript::data arrays

This part I imagine to be the most controversial. I think our current approach of exposing ScopeArray and friends is a poor idea, so something more generic like mozilla::Span seems better. Having support for range-based for loops makes code conceptually much cleaner. Decoupling from ScopeArray  makes changing the storage much easier (which the next patch does).

One problem with mozilla::Span is it currently uses more release asserts than we currently do. If specific cases are perf-critical, we can bypass these asserts with data()[index]. Another option would be our own form of span/slice for JS.

Jan, any thoughts or concerns about this direction?

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9003943 [details] [diff] [review]
Part 3: Use mozilla::Span for JSScript::data arrays

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

Makes sense to me. Iterating TryNotes is much nicer now!

::: js/src/vm/Interpreter.h
@@ +393,5 @@
>      {
>          if (script->hasTrynotes()) {
> +            auto trynotes = script->trynotes();
> +            tn_ = trynotes.data();
> +            tnEnd_ = tn_ + trynotes.size();

Here (and maybe elsewhere) we could also use trynotes.begin()/trynotes.end(), I think?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] from comment #8)
> Another option would be our own form of
> span/slice for JS.

I think mozilla::Range is less release-assert heavy but mozilla::Span is probably preferred for new code? When I skimmed the patch I didn't see any uses where I expect release asserts to be an issue though.

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Part 3: Use mozilla::Span for JSScript::data arrays

Replace various custom data-types in JSScript interfaces with
mozilla::Span. This abstracts implementation details and supports
range-based for-loops. Underlying storage is unchanged, but this sets us
up to be able to more easily change it.

MozReview-Commit-ID: FDfIYsAxTA8

Comment 12

[Ted Campbell [:tcampbell]]

Attachment: Part 3: Use mozilla::Span for JSScript::data arrays

Replace various custom data-types in JSScript interfaces with
mozilla::Span. This abstracts implementation details and supports
range-based for-loops. Underlying storage is unchanged, but this sets us
up to be able to more easily change it.

MozReview-Commit-ID: FDfIYsAxTA8

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: Part 4: Add js::PrivateScriptData type

This will later be used to store variable length data that hangs off of
each JSScript. This primarily is a refactor of existing JSScript code to
put in its own data structure.

- ScopeArray and friends now store offsets instead of pointers. This
  saves memory on 64-bit platforms and simplifies cloning.
- GCPtr constructors are used instead of relying on pod_calloc. This
  fixes C++ object-model violations.
- A packed bitfield is used to locate optional array headers instead of
  previous daisy-chain approach. This also lets js::PrivateScriptData
  understand array layout without coupling to JSScript.

MozReview-Commit-ID: AeSyTSPFtJY

Comment 14

[Ted Campbell [:tcampbell]]

Attachment: Part 5: Use js::PrivateScriptData for JSScript

- This makes JSScript::data arrays read-only. Initialization code
 directly uses PrivateScriptData to mutate.

MozReview-Commit-ID: LJFc8QazLfq

Comment 15

[Ted Campbell [:tcampbell]]

Attachment: Part 6: Cleanup js::SharedScriptData construction

Make SharedScriptData construction similar to PrivateScriptData.

MozReview-Commit-ID: 8tISXdbHobF

Comment 16

[Ted Campbell [:tcampbell]]

Attachment: Part 7: Cleanup JSScript initialization

- Remove CompileOptions check from JSScript constructor. Push up to
  JSScript::Call until further cleanup is done.
- Add createPrivateScriptData/createSharedScriptData helper functions.

MozReview-Commit-ID: 85ySh7mOnAt

Comment 17

[Ted Campbell [:tcampbell]]

Attachment: Part 5: Use js::PrivateScriptData for JSScript

- This makes JSScript::data arrays read-only. Initialization code
 directly uses PrivateScriptData to mutate.

MozReview-Commit-ID: LJFc8QazLfq

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 9003942 [details] [diff] [review]
Part 2: Remove CheckScriptDataIntegrity.

This doesn't catch anything on crashstats so remove since it depends on JSScript implementation details.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6aa090e485e
Part 1: Remove nTypeSets argument from JSScript::partiallyInit. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09a2762f0e5
Part 2: Remove CheckScriptDataIntegrity. r=jandem

Comment 20

[Ted Campbell [:tcampbell]]

Landing the trivial cleanups to get out of the way. Parts 3-7 will have the same structure, but I'm still doing small iterations on their design.

Comment 21

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a6aa090e485e
https://hg.mozilla.org/mozilla-central/rev/b09a2762f0e5

Comment 22

[Eric Rahm [:erahm]]

Ted, how big of an impact do you expect this to have on overhead? Or is it just a first step allowing us to work on other overhead bugs?

Comment 23

[Ted Campbell [:tcampbell]]

This is just mainly the first step. There may be some small wins on 64-bit (the AWSY runs I did were too noisy to tell).

Comment 24

[Ted Campbell [:tcampbell]]

Attachment: Part 3: Use mozilla::Span for JSScript::data arrays

Replace various custom data-types in JSScript interfaces with
mozilla::Span. This abstracts implementation details and supports
range-based for-loops. Underlying storage is unchanged, but this sets us
up to be able to more easily change it.

MozReview-Commit-ID: FDfIYsAxTA8

Comment 25

[Ted Campbell [:tcampbell]]

Attachment: Part 4: Add js::PrivateScriptData type

This will later be used to store variable length data that hangs off of
each JSScript. This primarily is a refactor of existing JSScript code to
put in its own data structure.

- ScopeArray and friends now store offsets instead of pointers. This
  saves memory on 64-bit platforms and simplifies cloning.
- GCPtr constructors are used instead of relying on pod_calloc. This
  fixes C++ object-model violations.
- A packed bitfield is used to locate optional array headers instead of
  previous daisy-chain approach. This also lets js::PrivateScriptData
  understand array layout without coupling to JSScript.

MozReview-Commit-ID: AeSyTSPFtJY

Comment 26

[Ted Campbell [:tcampbell]]

Attachment: Part 5: Use js::PrivateScriptData for JSScript

- This makes JSScript::data arrays read-only. Initialization code
 directly uses PrivateScriptData to mutate.

MozReview-Commit-ID: LJFc8QazLfq

Comment 27

[Ted Campbell [:tcampbell]]

Attachment: Part 6: Cleanup js::SharedScriptData construction

Make SharedScriptData construction similar to PrivateScriptData.

MozReview-Commit-ID: 8tISXdbHobF

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 9010252 [details] [diff] [review]
Part 3: Use mozilla::Span for JSScript::data arrays

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

Very nice.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +9670,2 @@
>  {
> +    MOZ_ASSERT(length() == array.size());

There's also array.Length() but that has the weird Gecko naming ;) size is a bit ambiguous in my mind (number of bytes vs items) but it's what the std lib uses so I guess it makes sense to use it.

::: js/src/vm/Realm.cpp
@@ +707,5 @@
>  static bool
>  AddInnerLazyFunctionsFromScript(JSScript* script, AutoObjectVector& lazyFunctions)
>  {
>      if (!script->hasObjects()) {
>          return true;

Not for this patch, but what do you think about returning an empty Span if !hasObjects() (and similar for other hasFoo) if it's easy to do?

Comment 29

[Jan de Mooij [:jandem]]

Comment on attachment 9010254 [details] [diff] [review]
Part 4: Add js::PrivateScriptData type

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

Looks good, but I'm glad you asked Waldo for feedback because I'm not sure about potential UB issues.

::: js/src/vm/JSScript.cpp
@@ +2683,5 @@
> +{
> +    size_t size = sizeof(PrivateScriptData);
> +
> +    // Pad scope array to maintain alignment. See: PrivateScriptData()
> +    size += nscopes * sizeof(GCPtrScope);

Please add a comment explaining why we can't (integer) overflow in this method.

@@ +2816,5 @@
> +    size_t size = AllocSize(nscopes, nconsts, nobjects,
> +                            ntrynotes, nscopenotes, nyieldoffsets);
> +
> +    // Allocate contiguous raw buffer
> +    uint8_t* raw = cx->pod_malloc<uint8_t>(JS_ROUNDUP(size, Alignment));

There's also pod_malloc_with_extra, but maybe that's more annoying to use here?

(We have more of these "C++ classes with variable-length data/arrays appended to it". I wonder if it's worth using templates/macros/codegen for it at some point in the future, but maybe that's overengineering.)

::: js/src/vm/JSScript.h
@@ +879,5 @@
> +        return mozilla::MakeSpan(base, span->length);
> +    }
> +
> +    // Helpers for creating initializing trailing data
> +    static uint32_t takeSpan(size_t& cursor);

Nit: s/size_t&/size_t*/ here and below?

@@ +924,5 @@
> +    bool hasConsts()       const { return bool(packedOffsets.constsSpanOffset); }
> +    bool hasObjects()      const { return bool(packedOffsets.objectsSpanOffset); }
> +    bool hasTryNotes()     const { return bool(packedOffsets.tryNotesSpanOffset); }
> +    bool hasScopeNotes()   const { return bool(packedOffsets.scopeNotesSpanOffset); }
> +    bool hasYieldOffsets() const { return bool(packedOffsets.yieldOffsetsSpanOffset); }

Nit: fine to keep as is, but personally I'm not a fan of aligning these things because renames or new methods can easily break it.

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 9010256 [details] [diff] [review]
Part 5: Use js::PrivateScriptData for JSScript

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

So nice to have this data in its own class instead of JSScript.

::: js/src/vm/JSScript.cpp
@@ +412,5 @@
>          }
>          if (script->hasObjects()) {
>              nobjects = script->objects().size();
>          }
>          nscopes = script->scopes().size();

Can remove this line because you moved it up.

@@ +2988,5 @@
>  
> +    PrivateScriptData* data;
> +    uint32_t dataSize;
> +
> +    data = PrivateScriptData::new_(cx, nscopes, nconsts, nobjects,

Nit: PrivateScriptData* data = ... (maybe \n after = if it's nicer).

@@ +3779,2 @@
>          for (unsigned i = 0; i < nconsts; ++i) {
> +            MOZ_ASSERT_IF(array[i].isGCThing(), array[i].toString()->isAtom());

Pre-existing but I think the point of this assert is that we don't need a post barrier for atoms (because they're never nursery-allocated), so we can skip the .init() in that case. Maybe add a comment to that effect?

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 9010258 [details] [diff] [review]
Part 6: Cleanup js::SharedScriptData construction

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

Looks good, but it's hard to review this because JSScript init is tricky and there are so many fields to consider.

::: js/src/vm/JSScript.cpp
@@ +3037,5 @@
>      jsbytecode* code = script->code();
>      code[0] = JSOP_RETRVAL;
> +
> +    jssrcnote* notes = script->notes();
> +    notes[0] = SRC_NULL;

Oh nice catch.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 9010254 [details] [diff] [review]
Part 4: Add js::PrivateScriptData type

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

::: js/src/vm/JSScript.cpp
@@ +2684,5 @@
> +    size_t size = sizeof(PrivateScriptData);
> +
> +    // Pad scope array to maintain alignment. See: PrivateScriptData()
> +    size += nscopes * sizeof(GCPtrScope);
> +    size = JS_ROUNDUP(size, alignof(GCPtrValue));

Seems to me that this should be right before nconsts, paired with that code.  And -- actually *in* the |if (nconsts)| block, I think you can do it, since it's only needed for that.

@@ +2706,5 @@
> +    return size;
> +}
> +
> +/* static */ uint32_t
> +PrivateScriptData::takeSpan(size_t& cursor)

This function isn't a bad idea.  But it's only called in the PrivateScriptData constructor.  There's no reason for it to be a class member function, and readability argues for it *not* being one.  Define a

  auto TakeSpan = [](size_t* cursor) {
    ..
  };

inside the constructor to do this.

@@ +2709,5 @@
> +/* static */ uint32_t
> +PrivateScriptData::takeSpan(size_t& cursor)
> +{
> +    MOZ_ASSERT(cursor % PackedOffsets::SCALE == 0);
> +    size_t packedOffset = cursor / PackedOffsets::SCALE;

inline PackedOffset
ToPackedOffset(size_t offset)
{
  return PackedOffset(cursor / PackedOffsets::SCALE);
}

to enforce proper typing on conversions.

@@ +2727,5 @@
> +    T* array = reifyOffset<T>(offset);
> +    MOZ_ASSERT(uintptr_t(array) % alignof(T) == 0);
> +
> +    for (uint32_t i = 0; i < length; ++i) {
> +        new (array + i) T;

Technically you should have

  uintptr_t array = reinterpret_cast<uintptr_t>(reifyOffset<T>(offset));

and then here

  new (reinterpret_cast<T*>(array + i * sizeof(T))) T;

to avoid adding to a pointer that doesn't actually contain its pointee type.

@@ +2755,5 @@
> +
> +// Initialize header and PackedSpans
> +PrivateScriptData::PrivateScriptData(uint32_t nscopes_, uint32_t nconsts, uint32_t nobjects,
> +                                     uint32_t ntrynotes, uint32_t nscopenotes, uint32_t nyieldoffsets)
> +  : packedOffsets{},

Could be worth a |= {}; // zeroes| at the declaration to avoid mentioning this to start.

@@ +2763,5 @@
> +    static_assert(alignof(PrivateScriptData) >= alignof(PackedSpan),  "Incompatible alignment");
> +    static_assert(alignof(PackedSpan)        >= alignof(GCPtrScope),  "Incompatible alignment");
> +    // NOTE: We manually pad between scopes and consts. This allows scope array
> +    // to come first and allow its offset to be encoded within PackedOffsets.
> +    static_assert(alignof(PrivateScriptData) >= alignof(GCPtrValue),  "Incompatible alignment");

These three lines especially make sense inside the

  if (nconsts) {
    ...
  }

block.

@@ +2767,5 @@
> +    static_assert(alignof(PrivateScriptData) >= alignof(GCPtrValue),  "Incompatible alignment");
> +    static_assert(alignof(GCPtrValue)        >= alignof(GCPtrObject), "Incompatible alignment");
> +    static_assert(alignof(GCPtrObject)       >= alignof(JSTryNote),   "Incompatible alignment");
> +    static_assert(alignof(JSTryNote)         >= alignof(ScopeNote),   "Incompatible alignment");
> +    static_assert(alignof(ScopeNote)         >= alignof(uint32_t),    "Incompatible alignment");

I'd prefer if these static_asserts were interspersed with the relevant computations below.

@@ +2770,5 @@
> +    static_assert(alignof(JSTryNote)         >= alignof(ScopeNote),   "Incompatible alignment");
> +    static_assert(alignof(ScopeNote)         >= alignof(uint32_t),    "Incompatible alignment");
> +
> +    // Header
> +    size_t cursor = sizeof(*this);

Maybe

  // Variable-length data begins immediately after PrivateScriptData itself.

instead of describing it as a header.  The meaning of |cursor| is the offset of the next space after what's been filled, basically, and "Header" doesn't really well describe that sense, or why you'd be initializing to this value.

@@ +2788,5 @@
> +
> +        initElements<GCPtrScope>(cursor, nscopes);
> +
> +        cursor += nscopes * sizeof(GCPtrScope);
> +        cursor = JS_ROUNDUP(cursor, alignof(GCPtrValue));

if (nconsts) {
  cursor = JS_ROUNDUP(cursor, alignof(GCPtrValue));
  initSpan<GCPtrValue>(..., packedOffsets.constsSpanOffset, ...);
}

@@ +2816,5 @@
> +    size_t size = AllocSize(nscopes, nconsts, nobjects,
> +                            ntrynotes, nscopenotes, nyieldoffsets);
> +
> +    // Allocate contiguous raw buffer
> +    uint8_t* raw = cx->pod_malloc<uint8_t>(JS_ROUNDUP(size, Alignment));

I don't think you need the alignment thing here.  You need an allocation adequate to hold the whole thing...but nothing is touching those trailing bits, it would be fine to allocate something small enough in them, so no need to round up.  Mild preference for |void*| for the type, to force translation of it to a usable type first.

@@ +2826,5 @@
> +    if (dataSize) {
> +        *dataSize = size;
> +    }
> +
> +    // Constuct the PrivateScriptData. Trailing arrays are unitialized but

uninitialized

::: js/src/vm/JSScript.h
@@ +802,5 @@
> +//
> +// The variable-length data associated with a script is stored in arrays
> +// following this data structure. This header describes which optional span
> +// structures exist, and those in turn describe each variable-length array.
> +// This indirection reduces the memory usage when these arrays are empty.

This comment isn't quite helpful enough in clearly laying out the memory representation.  I propose:

"""
PrivateScriptData stores variable-length data associated with a script.  Abstractly a PrivateScriptData consists of all these arrays:

  * a possibly-empty array of GCPtrScope in scopes(),
  * a possibly-empty array of GCPtrValue in consts(),
  * a possibly-empty array of JSObject* in objects(),
  * a possibly-empty array of JSTryNote in tryNotes(),
  * a possibly-empty array of ScopeNote in scopeNotes(),
  * a possibly-empty array of uint32_t in yieldAndAwaitOffsets()

Accessing any of these arrays just requires calling the appropriate public Span-computing function.

Under the hood, PrivateScriptData is a small class followed by a memory layout that compactly encodes all these arrays, in this manner (only explicit padding, "--" separators for readability only):

  <PrivateScriptData itself>
  --
  (OPTIONAL) PackedSpan for consts()
  (OPTIONAL) PackedSpan for objects()
  (OPTIONAL) PackedSpan for tryNotes()
  (OPTIONAL) PackedSpan for scopeNotes()
  (OPTIONAL) PackedSpan for yieldAndAwaitOffsets()
  --
  (OPTIONAL) all the GCPtrScopes that constitute scopes()
  --
  (OPTIONAL, if there are consts()) padding needed for space so far to be GCPtrValue-aligned
  (OPTIONAL) all the GCPtrValues that constitute consts()
  --
  (OPTIONAL) all the GCPtrObjects that constitute objects()
  (OPTIONAL) all the JSTryNotes that constitute tryNotes()
  (OPTIONAL) all the ScopeNotes that constitute scopeNotes()
  (OPTIONAL) all the uint32_t's that constitute yieldAndAwaitOffsets()

The contents of PrivateScriptData indicate which optional items are present.  PrivateScriptData::packedOffsets contains bit-fields, one per array.  Multiply each packed offset by sizeof(uint32_t) to compute a *real* offset.

PrivateScriptData::scopesOffset indicates where scopes() begins.  (Because scopes() elements are preceded by PrivateScriptData and up to five PackedSpans, its value is in the range [2, 7].)  PrivateScriptData::nscopes indicates the number of GCPtrScopes in scopes().

The other PackedScriptData::*Offset fields indicate where a potential corresponding PackedSpan resides.  If the packed offset is 0, there is no PackedSpan, and the array is empty.  Otherwise the PackedSpan's uint32_t offset and length fields store: 1) a *non-packed* offset (a literal count of bytes offset from the start of PrivateScriptData) to the corresponding array, and 2) the number of elements in the array, respectively.

PrivateScriptData and PackedSpan are 64-bit-aligned, so manual alignment in trailing fields is only necessary before the first trailing fields with increased alignment -- before GCPtrValues for consts(), on 32-bit, where the preceding GCPtrScopes as pointers are only 32-bit-aligned.
"""

Once consts are in SSD, alignment can be more simply addressed by just saying items are ordered from most aligned to least aligned.

@@ +804,5 @@
> +// following this data structure. This header describes which optional span
> +// structures exist, and those in turn describe each variable-length array.
> +// This indirection reduces the memory usage when these arrays are empty.
> +//
> +//  [Element Type]      [Accessor]

This table doesn't make any sense to me, TBH.

@@ +825,5 @@
> +{
> +    struct PackedOffsets
> +    {
> +        static constexpr unsigned SCALE = sizeof(uint32_t);
> +        static constexpr unsigned MAX_OFFSET = JS_BITMASK(4);

Not size_t for these?  Also the latter is clearer now as 0b1111 IMO.

@@ +835,5 @@
> +        uint32_t constsSpanOffset : 4;
> +        uint32_t objectsSpanOffset : 4;
> +        uint32_t tryNotesSpanOffset : 4;
> +        uint32_t scopeNotesSpanOffset : 4;
> +        uint32_t yieldOffsetsSpanOffset : 4;

I'd add

  enum class PackedOffset : uint32_t;

  inline size_t
  ToOffset(PackedOffset packed)
  {
    return size_t(packed) * sizeof(uint32_t);
  }

to make the conversion explicit and mandatory.  Extra compile-time units always help with rigor.

@@ +839,5 @@
> +        uint32_t yieldOffsetsSpanOffset : 4;
> +    };
> +
> +    // Detect accidental size regressions.
> +    static_assert(sizeof(PackedOffsets) == sizeof(uint32_t), "Unexpected bitfield packing");

I don't know if we've precisely formalized this, but as with our wrapping comments at 79ch, I find it's nice to wrap static_assert reasons at 79ch as well.  More readable like for comments, and for the same reasons.

@@ +851,5 @@
> +    };
> +
> +    // Concrete Fields
> +    PackedOffsets   packedOffsets;
> +    uint32_t        nscopes;

Don't align field names, it's just future-looking nuisance.

@@ +858,5 @@
> +    template <typename T>
> +    T* reifyOffset(size_t offset)
> +    {
> +        auto base = reinterpret_cast<uint8_t*>(this);
> +        return reinterpret_cast<T*>(base + offset);

Because |this| is not actually uint8_t, I think this doesn't work.  Adding and subtracting pointers requires that they point at actually valid stuff.  (Nor does [basic.lval]p11 exempt you, under a "you can access anything as unsigned char" theory -- the must-point-at limitation is intrinsic to the additive operators' semantics and does not invoke the char/unsigned char exception.)

You need to do the math in integral types, not pointers, to evade this.  So I think you need to do

        auto base = reinterpret_cast<uintptr_t>(this);

        MOZ_ASSERT((offset % alignof(T)) == 0, "offset must be T-aligned");
        return reinterpret_cast<T*>(base + offset);

instead.

I would probably name this offsetToPointer or something; reify is jargony and I have to think about what it means when I read it.

@@ +863,5 @@
> +    }
> +
> +    // Translate a PackedOffsets member into a pointer.
> +    template <typename T>
> +    T* unpackOffset(size_t scaledOffset)

packedOffsetToPointer?

@@ +871,5 @@
> +
> +    // Translates a PackedOffsets member into a PackedSpan* and then unpacks
> +    // that to a mozilla::Span.
> +    template <typename T>
> +    mozilla::Span<T> unpackSpan(size_t scaledSpanOffset)

packedOffsetToSpan?  You can trim the parameter name, too, with the typed enum being added.

@@ +874,5 @@
> +    template <typename T>
> +    mozilla::Span<T> unpackSpan(size_t scaledSpanOffset)
> +    {
> +        auto span = unpackOffset<PackedSpan>(scaledSpanOffset);
> +        auto base = reifyOffset<T>(span->offset);

The RHSes aren't an easily recognized idiom with obvious type.  Do

  PackedSpan* span = packedOffsetToPointer<PackedSpan>(offset);
  T* base = offsetToPointer<T>(span->offset);

instead which is far more readable.

@@ +889,5 @@
> +    void initElements(size_t offset, size_t length);
> +
> +    // Size to allocate
> +    static size_t AllocSize(uint32_t nscopes, uint32_t nconsts, uint32_t nobjects,
> +                            uint32_t ntrynotes, uint32_t nscopenotes, uint32_t nyieldoffsets);

allocationSize, or allocatedSize, is a better name IMO.  This name suggests allocation *of* a size.

@@ +899,5 @@
> +  public:
> +
> +    // Accessors for typed array spans.
> +    mozilla::Span<GCPtrScope> scopes() {
> +        auto base = unpackOffset<GCPtrScope>(packedOffsets.scopesOffset);

Don't use auto here.

@@ +919,5 @@
> +        return unpackSpan<uint32_t>(packedOffsets.yieldOffsetsSpanOffset);
> +    }
> +
> +    // Fast tests for if array exists
> +    bool hasScopes()       const { return true; }

It's probably better to not have to loop over a 0-length Span<GCPtrScope>, and to not have to explain an inconsistency -- nscopes > 0?  Unless there's always a scope, in which case that should be asserted in allocationSize and constructor both, and this function should be removed.

@@ +924,5 @@
> +    bool hasConsts()       const { return bool(packedOffsets.constsSpanOffset); }
> +    bool hasObjects()      const { return bool(packedOffsets.objectsSpanOffset); }
> +    bool hasTryNotes()     const { return bool(packedOffsets.tryNotesSpanOffset); }
> +    bool hasScopeNotes()   const { return bool(packedOffsets.scopeNotesSpanOffset); }
> +    bool hasYieldOffsets() const { return bool(packedOffsets.yieldOffsetsSpanOffset); }

Yeah, don't align goo.

Comment 33

[Ted Campbell [:tcampbell]]

Comment on attachment 9010252 [details] [diff] [review]
Part 3: Use mozilla::Span for JSScript::data arrays

Clearing review until I do re-write that Waldo asked for.

Comment 34

[Ted Campbell [:tcampbell]]

Comment on attachment 9010252 [details] [diff] [review]
Part 3: Use mozilla::Span for JSScript::data arrays

r=jandem from Comment 28. I'm dumb and cleared the wrong patch.

Comment 35

[Ted Campbell [:tcampbell]]

Comment on attachment 9010254 [details] [diff] [review]
Part 4: Add js::PrivateScriptData type

I need to re-write with Waldo's feedback

Comment 36

[Ted Campbell [:tcampbell]]

Comment on attachment 9010258 [details] [diff] [review]
Part 6: Cleanup js::SharedScriptData construction

Same concerns with Part 4 also apply here.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d60776b5be
Part 3: Use mozilla::Span for JSScript::data arrays. r=jandem

Comment 38

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/78d60776b5be

Comment 39

[Ted Campbell [:tcampbell]]

Attachment: Bug-1485347-Part-4-Add-js-PrivateScriptData-type.patch

- Main comment replaced by something closer to your suggestion
- No more jargon-y method names
- Lambdas instead of methods where appropriate
- No more pointer arithmetic on uninitialized memory

Comment 40

[Ted Campbell [:tcampbell]]

Attachment: Part 4: Add js::PrivateScriptData type

Same as Comment 39, but adds DefaultInitializeElements refactor (to be used again in part 6).

Comment 41

[Ted Campbell [:tcampbell]]

Attachment: Part 5: Use js::PrivateScriptData for JSScript

r=jandem from Comment 30 with nits addressed and rebasing.

Comment 42

[Ted Campbell [:tcampbell]]

Comment on attachment 9020256 [details] [diff] [review]
Part 4: Add js::PrivateScriptData type

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

::: js/src/vm/JSScript.h
@@ +1278,5 @@
> +//
> +// PrivateScriptData stores variable-length data associated with a script.
> +// Abstractly a PrivateScriptData consists of all these arrays:
> +//
> +//   * A possibly-empty array of GCPtrScope in scopes()

Self-nit: The scopes() array is not optional.

Comment 43

[Ted Campbell [:tcampbell]]

Comment on attachment 9010258 [details] [diff] [review]
Part 6: Cleanup js::SharedScriptData construction

Removing Part6 from this bug. Will open a new bug for more jsscript cleanups instead.

Comment 44

[Jeff Walden [:Waldo]]

Comment on attachment 9020256 [details] [diff] [review]
Part 4: Add js::PrivateScriptData type

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

::: js/src/vm/JSScript.cpp
@@ +3020,5 @@
> +static void
> +DefaultInitializeElements(void* arrayPtr, size_t length)
> +{
> +    uintptr_t array = reinterpret_cast<uintptr_t>(arrayPtr);
> +    MOZ_ASSERT(array % alignof(T) == 0);

Could name this elem, then add |elem += sizeof(T)| to the update-expression in the for-loop head.  Seems better than fully recomputing an address every single time, IMO in that the "striding" nature of the value is clearer than a multiply-named |i| calculation would make clear.

@@ +3055,5 @@
> +    if (nyieldoffsets) {
> +        size += sizeof(PackedSpan) + nyieldoffsets * sizeof(uint32_t);
> +    }
> +
> +    return size;

This is fine, but a part of me remains leery about not writing this out exactly in order as the fill-the-whole-thing code does it for direct correspondence.  Just sayin'.  :-|

::: js/src/vm/JSScript.h
@@ +1326,5 @@
> +// The other PackedScriptData::*Offset fields indicate where a potential
> +// corresponding PackedSpan resides. If the packed offset is 0, there is no
> +// PackedSpan, and the array is empty. Otherwise the PackedSpan's uint32_t
> +// offset and length fields store: 1) a *non-packed* offset (a literal count of
> +// bytes offset from the start of PrivateScriptData) to the corresponding

Emphasize *start* to be clear that 1) yes, it's from start and not end, and 2) this really is not a typo.  Reading this just now it looked wrong in the sense of "wouldn't from-end be more sensible" except I vaguely recalled from-start was going to be faster to compute because you wouldn't have the initial sizeof(PSD) to add.  Extra emphasis should help with making this particular "quirk" clearer and more obviously deliberate and intended.

@@ +1353,5 @@
> +    };
> +
> +    // Detect accidental size regressions.
> +    static_assert(sizeof(PackedOffsets) == sizeof(uint32_t),
> +                  "Unexpected bitfield packing");

static_assert reasons generally aren't capitalized.  And "bit-field" is the proper spelling of the terminology as the spec uses it.

@@ +1378,5 @@
> +    }
> +
> +    // Translate a PackedOffsets member into a pointer.
> +    template <typename T>
> +    T* packedOffsetToPointer(size_t packedOffset)

As I read all this again, I get a little scared about all the untyped size_t of different dimensionality floating around.  Could we do a *followup* that makes all the PackedOffsets::*Offset fields private, adds accessors that perform interconversions to/from a fresh

  enum class ScaledOffset : uint8_t;

type, and rename packedOffsetToPointer to scaledOffsetToPointer and change its argument to |ScaledOffset scaledOffset| and its body to |return offsetToPointer<T>(uint8_t(scaledOffset) * PackedOffsets::SCALE);|?  Also introduce |ScaledOffset| into the code that actually does the filling-in of a freshly allocated PSD.  That'll at least mean the scaled offsets can't be passed to something that accepts a normal offset, and vice versa, and some explicit conversion step must occur to get a normal offset from a scaled offset.

@@ +1436,5 @@
> +    bool hasConsts() const { return bool(packedOffsets.constsSpanOffset); }
> +    bool hasObjects() const { return bool(packedOffsets.objectsSpanOffset); }
> +    bool hasTryNotes() const { return bool(packedOffsets.tryNotesSpanOffset); }
> +    bool hasScopeNotes() const { return bool(packedOffsets.scopeNotesSpanOffset); }
> +    bool hasYieldOffsets() const { return bool(packedOffsets.yieldOffsetsSpanOffset); }

Mild preference for != 0 for these, given this more simply aligns with the doc-comment above in talking about zero and non-zero.

Comment 45

[Ted Campbell [:tcampbell]]

Attachment: Part 4: Add js::PrivateScriptData type

Carrying r=waldo from Comment 44.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31db69b4f38d
Part 4: Add js::PrivateScriptData type. r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbd4a0e4473
Part 5: Use js::PrivateScriptData for JSScript. r=jandem

Comment 47

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/31db69b4f38d
https://hg.mozilla.org/mozilla-central/rev/1fbd4a0e4473