1020455 - Reduce memory usage of LIR data structures

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attached are a few patches to shrink various LIR data structures a little.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: lir-fixedlist.patch

The number of LBlocks in a LIRGraph and the number of LPhis in a block are both fixed once set. This patch changes them to use a FixedList instead of a Vector.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: lir-osrblock.patch

This patch just removes the osrBlock_ field form LIRGraph, which was only used in one place and which was redundant with the osrBlock_ field in the MIRGraph.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: lir-phis.patch

This patch converts LBlock's phis_ array from being an array of pointers to individually allocated LPhis to being just an array of LPhis.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 8434287 [details] [diff] [review]
lir-fixedlist.patch

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

::: js/src/jit/LIR.h
@@ +747,5 @@
> +        LBlock *block = new(alloc) LBlock(from);
> +        size_t numLPhis = 0;
> +        for (MPhiIterator i(from->phisBegin()), e = from->phisEnd(); i != e; ++i)
> +            numLPhis += (i->type() == MIRType_Value) ? BOX_PIECES : 1;
> +        return block->phis_.init(alloc, numLPhis) ? block : nullptr;

Constructors this complicated are normally placed in the corresponding cpp file. Would you mind moving this to LIR.cpp?

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 8434292 [details] [diff] [review]
lir-phis.patch

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

Cool!

::: js/src/jit/LIR.cpp
@@ +71,5 @@
>      dump(stderr);
>  }
>  
> +LBlock *
> +LBlock::New(TempAllocator &alloc, MBasicBlock *from) {

uber-nit: { on new line.

@@ +93,5 @@
> +    // start of their defining block.
> +    size_t phiIndex = 0;
> +    size_t numPreds = from->numPredecessors();
> +    for (MPhiIterator i(from->phisBegin()), e(from->phisEnd()); i != e; ++i) {
> +        MPhi *phi = *i;

Just for sanity-checking, would you please MOZ_ASSERT(numPreds == phi->numOperands())?

@@ +96,5 @@
> +    for (MPhiIterator i(from->phisBegin()), e(from->phisEnd()); i != e; ++i) {
> +        MPhi *phi = *i;
> +        int numPhis = (phi->type() == MIRType_Value) ? BOX_PIECES : 1;
> +        for (int i = 0; i < numPhis; i++) {
> +            LAllocation *inputs = static_cast<LAllocation *>(alloc.allocateArray<sizeof(LAllocation)>(numPreds));

This line probably needs to be broken up for length. Temp var?

Comment 6

[Dan Gohman [:sunfish]]

(In reply to Sean Stangl [:sstangl] from comment #4)
> Comment on attachment 8434287 [details] [diff] [review]
> lir-fixedlist.patch
> 
> Review of attachment 8434287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/LIR.h
> @@ +747,5 @@
> > +        LBlock *block = new(alloc) LBlock(from);
> > +        size_t numLPhis = 0;
> > +        for (MPhiIterator i(from->phisBegin()), e = from->phisEnd(); i != e; ++i)
> > +            numLPhis += (i->type() == MIRType_Value) ? BOX_PIECES : 1;
> > +        return block->phis_.init(alloc, numLPhis) ? block : nullptr;
> 
> Constructors this complicated are normally placed in the corresponding cpp
> file. Would you mind moving this to LIR.cpp?

Done.

(In reply to Sean Stangl [:sstangl] from comment #5)
> Comment on attachment 8434292 [details] [diff] [review]
> lir-phis.patch
> 
> Review of attachment 8434292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool!
> 
> ::: js/src/jit/LIR.cpp
> @@ +71,5 @@
> >      dump(stderr);
> >  }
> >  
> > +LBlock *
> > +LBlock::New(TempAllocator &alloc, MBasicBlock *from) {
> 
> uber-nit: { on new line.

Fixed.

> @@ +93,5 @@
> > +    // start of their defining block.
> > +    size_t phiIndex = 0;
> > +    size_t numPreds = from->numPredecessors();
> > +    for (MPhiIterator i(from->phisBegin()), e(from->phisEnd()); i != e; ++i) {
> > +        MPhi *phi = *i;
> 
> Just for sanity-checking, would you please MOZ_ASSERT(numPreds ==
> phi->numOperands())?

Added.

> @@ +96,5 @@
> > +    for (MPhiIterator i(from->phisBegin()), e(from->phisEnd()); i != e; ++i) {
> > +        MPhi *phi = *i;
> > +        int numPhis = (phi->type() == MIRType_Value) ? BOX_PIECES : 1;
> > +        for (int i = 0; i < numPhis; i++) {
> > +            LAllocation *inputs = static_cast<LAllocation *>(alloc.allocateArray<sizeof(LAllocation)>(numPreds));
> 
> This line probably needs to be broken up for length. Temp var?

Temp var. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/48b40e6ca833
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a81052fe9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/daae873f90b3

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48b40e6ca833
https://hg.mozilla.org/mozilla-central/rev/81a81052fe9f
https://hg.mozilla.org/mozilla-central/rev/daae873f90b3