739512 - Shrink JSScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I have some patches that reduce the size of JSScript and also clean up its 
code, esp. the handling of JSScript::data.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Patch 1: remove cookie[12] and reorder JSScript members

This patch does two things:

- Removes JSScript::cookie[12] and related methods, because billm says
  they're no longer needed.

- Reorders the members within JSScript, so that (a) all fields are before
  all methods, and (b) fields are ordered from largest to smallest to ensure
  no space is wasted due to alignment.  Other than the ordering, I didn't
  change anything, except adding public/private qualifiers where necessary.
  (I also checked carefully that all members have the same public/private
  visibility that they did before the patch.)  Change (b) doesn't make
  JSScript any smaller, but it will help the subsequent patches that do.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Patch 2: space-optimize representation of closedArgs and closedVars

This patch uses the existing space-optimized "optional array" 
representation for a JSScript's closedArgs and closedVars.  This makes
sense because I found during normal browsing that only ~2% of all 
JSScripts had non-zero closedArgs and similar numbers for closedVars.

On 32-bit platforms, this reduces the size of JSScript from 128 to 120
bytes, increasing the number that can be fit in an arena from 31 to 34.

On 64-bit platforms, this reduces the size of JSScript from 200 to 192
bytes, increasing the number that can be fit in an arena from 20 to 21.

The patch also improves the documentation and static assertions for
JSScript::data's layout.  It also makes all the code that operates on
JSScript::data handle the arrays in the same order.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Patch 3: shrink the representation of optional arrays

This patch optimizes the representation of the optional arrays.  Previously,
for each optional array kind, JSScript held a uint8_t offset.  But you only
need to store a single present/not-present bit for each array kind;  from
that you can compute the possible offsets because the size of each array
header is known statically.  I've created some small lookup tables for every
possible combination of present/not-present arrays.

On 32-bit platforms, this reduces the size of JSScript from 120 to 112
bytes, increasing the number that can be fit in an arena from 34 to 36.

On 64-bit platforms, this reduces the size of JSScript from 192 to 184
bytes, increasing the number that can be fit in an arena from 21 to 22.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Patch 4: move JS{Const,Object,TryNote}Array into the |js| namespace

This patch is boring:  it just moves JSConstArray, JSObjectArray, and
JSTryNoteArray into the |js| namespace, to match GlobalSlotArray and
ClosedSlotArray.

Comment 5

[Luke Wagner [:luke]]

OOC, what is the overall size change?

Also, a small request: could you s/nClosedArgs/numClosedArgs/ and s/nClosedVars/numClosedVars/ for consistency with other num* methods?

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Luke Wagner [:luke] from comment #5)
> OOC, what is the overall size change?

For 32-bits:  128 --> 112 bytes, i.e. 31 --> 36 per arena
For 64-bits:  200 --> 184 bytes, i.e. 20 --> 22 per arena

> Also, a small request: could you s/nClosedArgs/numClosedArgs/ and
> s/nClosedVars/numClosedVars/ for consistency with other num* methods?

The only one I see is numNotes()... I was just reusing the existing nClosedArgs/nClosedVars names.  But sure, I can make the change.

Comment 7

[Luke Wagner [:luke]]

Great, thanks.  numX is used in a lot more places than jsscript.h.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 609599 [details] [diff] [review]
Patch 3: shrink the representation of optional arrays

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

What are the absolute savings with this patch, on average? I'm worried about the complexity of this patch, both the changes to jsscript.cpp and how difficult packing like this makes debugging structures in gdb.

Comment 9

[Nicholas Nethercote [inactive]]

> What are the absolute savings with this patch, on average? I'm worried about
> the complexity of this patch, both the changes to jsscript.cpp and how
> difficult packing like this makes debugging structures in gdb.

From comment 3:

> On 32-bit platforms, this reduces the size of JSScript from 120 to 112
> bytes, increasing the number that can be fit in an arena from 34 to 36.
> 
> On 64-bit platforms, this reduces the size of JSScript from 192 to 184
> bytes, increasing the number that can be fit in an arena from 21 to 22.

If you look at about:memory, the per-compartment "scripts" numbers are typically quite large, e.g. multiple MB in the larger compartments.  This patch will reduce that by ~5%.

The complexity doesn't worry me -- it's confined to jsscript.{h,cpp}, and the introduction of the functions like hasConsts() makes code that interacts with JSScript easier to read.

I'm more sympathetic to the GDB concerns.  Are there other helper functions I could add that would improve things?  Or would that be no help because they'd be inlined and thus not usable within GDB?

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Patch 5: remove JITScript::arityCheckEntry

JITScript::arityCheckEntry isn't used in any meaningful way.  I suspect it was used in the past but that JSScript::jitArityCheck{Normal,Ctor} replaced its function.

This patch removes it.  The change reduces the size of JITScript from 56 to 52 bytes on 32-bit, and from 104 to 96 bytes on 64-bit.

Comment 11

[Nicholas Nethercote [inactive]]

> I'm more sympathetic to the GDB concerns.  Are there other helper functions
> I could add that would improve things?  Or would that be no help because
> they'd be inlined and thus not usable within GDB?

So I tried GDB and the functions like consts(), objects() all work fine.  So it's no harder to access those arrays in the debugger.  dvander, do you have any specific other concerns?

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Patch 6: improving naming of PCCount-related stuff

I started looking at moving pcCounters into JSCompartment, but I found the
naming for the PCCount stuff confusing, so I thought I'd clean it up first.
|script.pcCounters->counts->counts|, yikes!  

This patch:

- Avoids inconsistent usage of "counts" and "counters", by using "counts"
  everywhere.

- Better distinguishes the different kinds of "counts" -- "OpcodeCounts"
  becomes "PCCounts" (to match the name of the API) and "ScriptOpcodeCounts"
  becomes "ScriptCounts".  Some other types and fields were renamed
  accordingly (e.g.  "ScriptOpcodeCountsPair" becomes "ScriptAndCounts").
  Generally, it's now more clear which kind of "counts" we're dealing with
  at any point.

- Renames the *_COUNT constants (e.g. BASE_COUNT) as *_LIMIT, to avoid
  yet another meaning of "count".

- Adds a comment describing the storage strategy used for ScriptCounts'
  vector (which has been renamed |pcCountsVec|).

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 609599 [details] [diff] [review]
Patch 3: shrink the representation of optional arrays

dvander, what should we do with patch 3... how about a second opinion?  Luke, please see comment 3, 8, 9 and 11 for context.

Comment 14

[Nicholas Nethercote [inactive]]

Patch 1 and 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4df291002dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f3d6a230512

Comment 15

[Nicholas Nethercote [inactive]]

Patch 5 and 6:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9b6bdf63f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3142f4268517

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Patch 7: move scriptCounts into a table

This patch moves scriptCounts out of JSScript and into 
JSCompartments::scriptCountsMap.  This makes sense because scriptCounts is
very often NULL and it doesn't require fast access.  (However, detecting if 
a JSScript has counts must be fast, and JSScript::hasCountsMap caters for 
that.)

On 32-bit platforms, this doesn't change the size of JSScript -- I had to add 32 bits of padding to keep the size a multiple of 8 (as is required for GC cells).

On 64-bit platforms, this reduces the size of JSScript from 184 to 176
bytes, increasing the number that can be fit in an arena from 22 to 23.

I tested this by running the js shell with -D on Sunspider.  Is that enough?

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Draft patch: move jitArityCheck into JITScript

This patch moves script->jitArityCheckNormal into script->jitNormal->jitArityCheck and script->jitArityCheckCtor into script->jitCtor->jitArityCheck.  This saves two words in JSScript, and because most JSScripts don't have JITScripts, it's a space win.

The most notable change is that generateFullCallStub() has to generate an extra dependent load.

Currently it doesn't quite work -- I get 7 failures in jit-tests, all crashes in generated code.  dvander, any suggestions on what I might be doing wrong, or comments on the general feasibility of this idea, would be welcome.  Thanks.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Patch 7, v2: move scriptCounts into a table

This version of patch 7 fixes some problems that Valgrind found.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: patch 8: move sourceMap into a table

This patch moves sourceMap out of JSScript and into
JSCompartments::sourceMapMap.  This is worthwhile because sourceMap is
very often NULL and it doesn't (AFAICT) require fast access.  
JSScript::hasCountsMap provides a simple way to test if a sourceMap is
present. 

On 32-bit platforms, this reduces the size of JSScript from 112 to 104
bytes, increasing the number that can be fit in an arena from 36 to 39.

On 64-bit platforms, this reduces the size of JSScript from 176 to 168
bytes, increasing the number that can be fit in an arena from 23 to 24.

Comment 20

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> dvander, what should we do with patch 3... how about a second opinion? 
> Luke, please see comment 3, 8, 9 and 11 for context.

I certainly like the new interface vs. the old isValidOffset funk.

If we go ahead later this year with the column info + save-the-source + decompiler removal + lazy compilation plan (they all seemed to support each other), then conceivably the memory pressure due to JSScript would be significantly reduced.  (One can even imagine that we start throwing away cold JSScripts during GC which would result in an even more severe reduction!)  In that case, these micro-optimizations (5% of ~5% of total explicit allocation) would not be worth our time or maintenance effort.

While I'm not thrilled about the re-emergence of the #define R macros, the patch isn't so complex that I'm absolutely opposed, so if you really care I'll review the patch.

Comment 21

[Nicholas Nethercote [inactive]]

> If we go ahead later this year with the column info + save-the-source +
> decompiler removal + lazy compilation plan (they all seemed to support each
> other), then conceivably the memory pressure due to JSScript would be
> significantly reduced.  (One can even imagine that we start throwing away
> cold JSScripts during GC which would result in an even more severe
> reduction!)  In that case, these micro-optimizations (5% of ~5% of total
> explicit allocation) would not be worth our time or maintenance effort.

I started looking at the lazy bytecode stuff and concluded that it's a big, complicated, high-risk change.  (Especially for me, as someone who doesn't know the front-end at all well.)  While looking I noticed some easy JSScript wins, so I decided to pursue them first.  I have some more patches coming, by the time I'm done I should have reduced sizeof(JSScript) by 25% or more.

More generally, JS engine memory consumption is dominated by objects, shapes, script and strings, so even small improvements in any of those can have sizeable effects.
 
> While I'm not thrilled about the re-emergence of the #define R macros, the
> patch isn't so complex that I'm absolutely opposed, so if you really care
> I'll review the patch.

The R macro is a hack, but it ended up being nicer than my first version which initialized the lookup tables on demand.  They are static data, so generating them statically feels appropriate.

So -- yes, please review!

Comment 22

[Jan de Mooij [:jandem]]

It looks like this regressed v8bench (http://arewefastyet.com/?view=regress), in particular v8-richards. Nicholas, would you mind taking a look?

Comment 23

[Nicholas Nethercote [inactive]]

(In reply to Jan de Mooij (:jandem) from comment #22)
> It looks like this regressed v8bench
> (http://arewefastyet.com/?view=regress), in particular v8-richards.
> Nicholas, would you mind taking a look?

Huh, that's a big regression.  It's late here, I'll take a look tomorrow.  Thanks for telling me!

Comment 24

[Boris Zbarsky [:bzbarsky]]

The regression also showed up on Dromaeo(v8) (22% regression on Linux on inbound!) and Dromaeo(jslib) (7% regression on Linux on inbound).

Looking into the latter might be worth it if it's specific to one of the subtests, since those might be smaller than all of v8-richards.

Do we need a separate bug open to track the regression, since this one is still tracking continuing work?

Comment 25

[Andrew McCreight [:mccr8]]

Part 5 and 6 show up as causing 30% v8 regressions on OSX, too.

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/9sahv0snBno

Comment 26

[Boris Zbarsky [:bzbarsky]]

Also regressions in the 3-4% range on Dromaeo(CSS).

Comment 27

[Matt Brubeck (:mbrubeck)]

1. https://hg.mozilla.org/mozilla-central/rev/f4df291002dd
2. https://hg.mozilla.org/mozilla-central/rev/2f3d6a230512

5. https://hg.mozilla.org/mozilla-central/rev/2f9b6bdf63f7
6. https://hg.mozilla.org/mozilla-central/rev/3142f4268517

Comment 28

[Nicholas Nethercote [inactive]]

I backed out patch 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9894cd999781

While removing the dead field I accidentally changed the code generated by JM.  Sorry!

Comment 29

[Nicholas Nethercote [inactive]]

Comment on attachment 611340 [details] [diff] [review]
Draft patch: move jitArityCheck into JITScript

I have a working version of this coming up, but I might use a different bug for it.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9894cd999781

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 611348 [details] [diff] [review]
patch 8: move sourceMap into a table

> JS_PUBLIC_API(const jschar *)
> JS_GetScriptSourceMap(JSContext *cx, JSScript *script)
> {
>-    return script->sourceMap;
>+    return script->getSourceMap();
> }
...
>+jschar *
>+JSScript::getSourceMap() {
>+    JS_ASSERT(hasSourceMap);

JS_GetScriptSourceMap is supposed to return NULL if !script->hasSourceMap, not assert/crash.

r=me with that fixed.

Comment 32

[Brian Hackett [Laid off!]]

Comment on attachment 611341 [details] [diff] [review]
Patch 7, v2: move scriptCounts into a table

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

::: js/src/jsscript.cpp
@@ +820,5 @@
>          next = pc + GetBytecodeLength(pc);
>      }
>  
> +    hasScriptCounts = true;
> +    map->putNew(this, scriptCounts);

I think this is fallible.

Comment 33

[Nicholas Nethercote [inactive]]

I patch two I failed to rename nClosed{Vars,Args} as Luke requested in comment 5.  This follow-up patch does that:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd73b2ca1fcc

Comment 34

[Nicholas Nethercote [inactive]]

Attachment: patch 9: move debug into a table

This patch is similar to patch 7 and 8 -- it moves |debug| out of JSScript
and into JSCompartments::debugScriptMap.  This is worthwhile because |debug|
is very often NULL and it doesn't (AFAICT?) require fast access.  
JSScript::hasDebugScript provides a simple way to test if a sourceMap is
present.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

njn, did you mean to ask someone to review?

Comment 36

[Nicholas Nethercote [inactive]]

Comment on attachment 613483 [details] [diff] [review]
patch 9: move debug into a table

Yes!  Thanks.

Comment 37

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/cd73b2ca1fcc

Comment 38

[Nicholas Nethercote [inactive]]

Patch 7:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd10e0e6e5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0feac51b6605
https://hg.mozilla.org/integration/mozilla-inbound/rev/9187a8ec5003

I botched the initial landing (I had an uncommitted change), then backed it out and relanded.

Comment 39

[Nicholas Nethercote [inactive]]

Patch 8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/30e9d2a85613

Comment 40

[Nicholas Nethercote [inactive]]

Attachment: patch 10: add JSScripts::hasConsts() et al

This patch just extracts the clean-ups from patch 3, which should be uncontroversial.

Comment 41

[Nicholas Nethercote [inactive]]

Attachment: Patch 4, v2: move JS{Const,Object,TryNote}Array into the |js| namespace

This is just like patch 4, but updated for the changes that have landed, and it doesn't need patch 3 to be applied.  Should be uncontroversial.

Comment 42

[Matt Brubeck (:mbrubeck)]

7. https://hg.mozilla.org/mozilla-central/rev/9187a8ec5003

Comment 43

[Nicholas Nethercote [inactive]]

Attachment: Patch 11: change JSScript::useCount to uint32_t

This patch changes JSScript::useCount from size_t to uint32_t.  On 32-bit 
platforms it makes no difference to JSScript's size.  On 64-bit platforms it 
potentially reduces JSScript's size... at this point in time it doesn't,
but if other changes happen it might.  For example, if patch 3 is
subsequently applied, the result is smaller than if useCount is a size_t.

Also, I find the existing code hard to understand in the 64-bit case.
methodjit/Compiler.cpp has this (this is pre-patch code):

    size_t *addr = script->addressOfUseCount();
    masm.add32(Imm32(1), AbsoluteAddress(addr));

and the generated code looks like this:

    [jaeger] Insns            movabsq    $0x7f82cce13218, %r11
    [jaeger] Insns            addl       1, 0x0(%r11)

Why are we doing a 32-bit add on a 64-bit value?  I *think* this only works
because x86-64 is little-endian, and the high 32-bits of useCount are
never touched.  Assuming that's right, IMO having a uint32_t and consistent 
code on 32-bit and 64-bit platforms makes things clearer.

Comment 44

[Nicholas Nethercote [inactive]]

Attachment: Patch 3, v2: shrink the representation of optional arrays

This is a revised version of patch 3 that's a lot simpler.  Computing the
static offset lookup table was a premature optimization;  Cachegrind (on
Sunspider) tells me the slowdown from computing the offsets at run-time is
truly negligible.  So I removed the lookup table.

In terms of how this compares to the existing code.

- Currently we have this custom, space-optimized representation, based on
  storing an offset for each array (or INVALID_OFFSET if not present).

  My patch changes this to a custom, space-optimized representation, based
  on storing a single bit for each array to indicate its presence, and
  computing the offset from these bits when necessary.

- From a debugging point of view, this change makes almost no difference,
  viz:

       js::ConstArray *consts() {
           JS_ASSERT(hasConsts());
  -        return reinterpret_cast<js::ConstArray *>(data + constsOffset);
  +        return reinterpret_cast<js::ConstArray *>(data + constsOffset());
       }

- The new version is simpler in some ways.  For example, we don't have to
  worry about whether uint8_t is big enough to hold all the array offsets,
  which eliminates the need for a couple of assertions.  Also, we don't have
  to set INVALID_OFFSET in the cases where the arrays aren't present.
  Overall it's a net reduction of 16 lines of code.

- When applied on top of the other outstanding patches in this bug, it
  reduces JSScript's size (and the number that can be fit per arena) as
  follows:

            old         new
  32-bit    104/39.2     96/42.5
  64-bit    152/26.7    144/28.2

  That's a 1.077x increase in the per-arena number (which is the important 
  one) in both cases.  (Before I started this JSScript shrinking work the
  per-arena numbers were 31 and 19.)

Comment 45

[Luke Wagner [:luke]]

Comment on attachment 614245 [details] [diff] [review]
Patch 3, v2: shrink the representation of optional arrays

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

Looks good

::: js/src/jsscript.h
@@ +735,5 @@
>      /* Script notes are allocated right after the code. */
>      jssrcnote *notes() { return (jssrcnote *)(code + length); }
>  
> +    #define HAS_ARRAY(v, kind)      ((v) &  (1 << (kind)))
> +    #define SET_HAS_ARRAY(v, kind)  ((v) |= (1 << (kind)))

Could these be real static functions instead?

@@ +747,5 @@
> +    bool hasClosedArgs()    { return HAS_ARRAY(hasArrayBits, CLOSED_ARGS); }
> +    bool hasClosedVars()    { return HAS_ARRAY(hasArrayBits, CLOSED_VARS); }
> +
> +    // The offset added for the "foo" array, which has element type |t|.
> +    #define OFF(fooOff, hasFoo, t)   (fooOff() + (hasFoo() ? sizeof(t) : 0))

For this one, it seems more readable to just straight-up inline.

Comment 46

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/30e9d2a85613

Comment 47

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/cdd78230db65

Comment 48

[Nicholas Nethercote [inactive]]

dvander: review ping.  (Patches 4v2 and 10 are trivial refactorings;  patch 11 is very short.)

Comment 49

[Nicholas Nethercote [inactive]]

Patch 9 (it landed a while ago, sorry for the late link):

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd78230db65

Comment 50

[Nicholas Nethercote [inactive]]

Patches 10 and 4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6551d2bc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0831ce6ba72f

Comment 51

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/34e6551d2bc1
https://hg.mozilla.org/mozilla-central/rev/0831ce6ba72f

Comment 52

[Nicholas Nethercote [inactive]]

Patch 11.  Almost there:

https://hg.mozilla.org/integration/mozilla-inbound/rev/390dc1d722ab

Comment 53

[Nicholas Nethercote [inactive]]

Patch 3:  Finished!

https://hg.mozilla.org/integration/mozilla-inbound/rev/25d54e0cdf31

Comment 54

[Nicholas Nethercote [inactive]]

And a bustage fix for patch 3, just for good luck.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a12879dc9776

Comment 55

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/390dc1d722ab
https://hg.mozilla.org/mozilla-central/rev/25d54e0cdf31
https://hg.mozilla.org/mozilla-central/rev/a12879dc9776

Comment 56

[Landry Breuil (:gaston)]

Very likely to be the cause of a new build failure on powerpc :

/home/landry/src/mozilla-central/js/src/jsscript.h:937: error: size of array 'arg' is negative

Last m-c build from a week ago on ppc didnt fail like this. Wild guess, i'd say either patch 3 or 11.

Comment 57

[Landry Breuil (:gaston)]

It looks like readding the following chunk :

+#if JS_BITS_PER_WORD == 32
+private:
+   uint32_t        pad32;
+#endif
+

removed in patch 3 fixes it (at least build under js/src goes further)

Comment 58

[Landry Breuil (:gaston)]

Looking a bit more closely, it seems the chunk was added in patch 9 and removed in patch 3, so i don't know what was the intent here..