Closed Bug 739512 Opened 12 years ago Closed 12 years ago

Shrink JSScript

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(10 files, 5 obsolete files)

17.84 KB, patch
billm
: review+
Details | Diff | Splinter Review
32.74 KB, patch
dvander
: review+
Details | Diff | Splinter Review
84.43 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
44.75 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
9.61 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.12 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
16.05 KB, patch
dvander
: review+
Details | Diff | Splinter Review
22.34 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.22 KB, patch
dvander
: review+
Details | Diff | Splinter Review
13.12 KB, patch
luke
: review+
Details | Diff | Splinter Review
I have some patches that reduce the size of JSScript and also clean up its 
code, esp. the handling of JSScript::data.
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.
Attachment #609597 - Flags: review?(wmccloskey)
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.
Attachment #609597 - Attachment is obsolete: true
Attachment #609597 - Flags: review?(wmccloskey)
Attachment #609598 - Flags: review?(dvander)
Attachment #609597 - Attachment is obsolete: false
Attachment #609597 - Flags: review?(wmccloskey)
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.
Attachment #609599 - Flags: review?(dvander)
This patch is boring:  it just moves JSConstArray, JSObjectArray, and
JSTryNoteArray into the |js| namespace, to match GlobalSlotArray and
ClosedSlotArray.
Attachment #609601 - Flags: review?(dvander)
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?
(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.
Great, thanks.  numX is used in a lot more places than jsscript.h.
Attachment #609598 - Flags: review?(dvander) → review+
Attachment #609597 - Flags: review?(wmccloskey) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
> 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?
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.
Attachment #610326 - Flags: review?(dvander)
> 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?
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|).
Attachment #610433 - Flags: review?(bhackett1024)
Attachment #610326 - Flags: review?(dvander) → review+
Attachment #610433 - Flags: review?(bhackett1024) → review+
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.
Attachment #609599 - Flags: review?(luke)
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?
Attachment #611337 - Flags: review?(bhackett1024)
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.
Attachment #611340 - Flags: feedback?(dvander)
This version of patch 7 fixes some problems that Valgrind found.
Attachment #611337 - Attachment is obsolete: true
Attachment #611337 - Flags: review?(bhackett1024)
Attachment #611341 - Flags: review?(bhackett1024)
Attachment #611340 - Attachment description: Patch 8 (draft): move jitArityCheck into JITScript → Draft patch: move jitArityCheck into JITScript
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.
Attachment #611348 - Flags: review?(jorendorff)
(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.
> 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!
It looks like this regressed v8bench (http://arewefastyet.com/?view=regress), in particular v8-richards. Nicholas, would you mind taking a look?
(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!
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?
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
Also regressions in the 3-4% range on Dromaeo(CSS).
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 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.
Attachment #611340 - Flags: feedback?(dvander)
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.
Attachment #611348 - Flags: review?(jorendorff) → review+
Attachment #610326 - Attachment is obsolete: true
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.
Attachment #611341 - Flags: review?(bhackett1024) → review+
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
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.
Attachment #613483 - Flags: review?
njn, did you mean to ask someone to review?
Comment on attachment 613483 [details] [diff] [review]
patch 9: move debug into a table

Yes!  Thanks.
Attachment #613483 - Flags: review? → review?(jorendorff)
Attachment #613483 - Flags: review?(jorendorff) → review+
This patch just extracts the clean-ups from patch 3, which should be uncontroversial.
Attachment #613885 - Flags: review?(dvander)
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.
Attachment #609601 - Attachment is obsolete: true
Attachment #609601 - Flags: review?(dvander)
Attachment #613887 - Flags: review?
Attachment #613887 - Flags: review? → review?(dvander)
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.
Attachment #614225 - Flags: review?(dvander)
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.)
Attachment #609599 - Attachment is obsolete: true
Attachment #609599 - Flags: review?(luke)
Attachment #609599 - Flags: review?(dvander)
Attachment #614245 - Flags: review?(luke)
Attachment #611341 - Attachment description: Patch 7b: move scriptCounts into a table → Patch 7, v2: move scriptCounts into a table
Attachment #613887 - Attachment description: Patch 4b: move JS{Const,Object,TryNote}Array into the |js| namespace → Patch 4, v2: move JS{Const,Object,TryNote}Array into the |js| namespace
Attachment #611340 - Attachment is obsolete: true
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.
Attachment #614245 - Flags: review?(luke) → review+
dvander: review ping.  (Patches 4v2 and 10 are trivial refactorings;  patch 11 is very short.)
Attachment #614225 - Flags: review?(dvander) → review+
Attachment #613885 - Flags: review?(dvander) → review+
Attachment #613887 - Flags: review?(dvander) → review+
Patch 9 (it landed a while ago, sorry for the late link):

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd78230db65
Patch 3:  Finished!

https://hg.mozilla.org/integration/mozilla-inbound/rev/25d54e0cdf31
Whiteboard: [MemShrink:P2][leave open] → [MemShrink:P2]
And a bustage fix for patch 3, just for good luck.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a12879dc9776
https://hg.mozilla.org/mozilla-central/rev/390dc1d722ab
https://hg.mozilla.org/mozilla-central/rev/25d54e0cdf31
https://hg.mozilla.org/mozilla-central/rev/a12879dc9776
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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.
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)
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..
Depends on: 754641
Depends on: 790146
Depends on: 787887
You need to log in before you can comment on or make changes to this bug.