Reduce memory usage of LIR data structures

RESOLVED FIXED in mozilla32

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Attached are a few patches to shrink various LIR data structures a little.
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.
Assignee: nobody → sunfish
Attachment #8434287 - Flags: review?(sstangl)
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.
Attachment #8434290 - Flags: review?(sstangl)
This patch converts LBlock's phis_ array from being an array of pointers to individually allocated LPhis to being just an array of LPhis.
Attachment #8434292 - Flags: review?(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?
Attachment #8434287 - Flags: review?(sstangl) → review+
Attachment #8434290 - Flags: review?(sstangl) → review+
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?
Attachment #8434292 - Flags: review?(sstangl) → review+
(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
You need to log in before you can comment on or make changes to this bug.