845175 - Lazily allocate the bytecode and srcnote Vectors' storage

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 843462 increased the "Trace Malloc Allocs" counts by 2.4--3.1% on four
platforms.  For example:

> Regression: Mozilla-Inbound - Trace Malloc Allocs - WINNT 5.2 - 3.1% increase
> -----------------------------------------------------------------------------
>     Previous: avg 455425.033 stddev 723.199 of 30 runs up to revision 924c07c3cb32
>     New     : avg 469559.600 stddev 514.259 of 5 runs since revision 10fad54535e7
>     Change  : +14134.567 (3.1% / z=19.545)
>     Graph   : http://mzl.la/WlMkeF

The problem is that bug 843462 eagerly allocates the bytecode and srcnote
vectors' storage, but it turns out that quite often those Vectors end up with
zero elements.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Lazily allocate the bytecode and srcnote Vectors' storage.

This patch makes the Vector storage creation lazy.  I used diagnostic printfs
to confirm that, with this patch, the number of bytecode/srcnote allocations
is identical to the old code.

It also removes the empty BytecodeEmitter destructor, which you asked me to do
in bug 843462 and I failed to.

Comment 2

[Nicholas Nethercote [inactive]]

jorendorff: review ping.  This is a tiny patch that fixes a Talos regression.

Comment 3

[:Benjamin Peterson]

When is there no bytecode?

Comment 4

[Nicholas Nethercote [inactive]]

> When is there no bytecode?

It's quite common.  We store bytecode for the prolog separately from the main section, so I'd guess that an empty prolog is common.

Comment 5

[:Benjamin Peterson]

Ah right; I forgot about the prologue. That should whole separation should be deleted someday.

Comment 6

[Nicholas Nethercote [inactive]]

jorendorff: second review ping.  This is a tiny patch that fixes a Talos regression.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 718224 [details] [diff] [review]
Lazily allocate the bytecode and srcnote Vectors' storage.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ -124,5 @@
>  
> -BytecodeEmitter::~BytecodeEmitter()
> -{
> -}
> -

Thank you.

@@ +129,5 @@
>  
> +    // Start it off moderately large to avoid repeated resizings early on.
> +    if (bce->code().capacity() == 0)
> +        bce->code().reserve(1024);
> +

If this reserve() call did fail, it would've already reported OOM, so I think it should be like:

    if (bce->code().capacity() == 0) {
        if (!bce->code().reserve(1024))
            return -1;
    }

I don't worry about magic numbers; if you wanted to make a const, whatever.

Same comments in AllocSrcNote.

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

[Nicholas Nethercote [inactive]]

I've confirmed this fixed the talos regression:

http://graphs.mozilla.org/graph.html#tests=[[30,63,18]]&sel=1361264223963,1363856223963&displayrange=30&datatype=running