889682 - GenerationalGC: Assertion failure: thing->isAligned(), at gc/Marking.cpp or Assertion failure: MapTypeToTraceKind<T>::kind == GetGCThingTraceKind(thing), at gc/Marking.cpp or Assertion failure: !InFreeList(thing->arenaHeader(), thing), at gc/Marking.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

gc();
var recursiveFunctions = [{
    text: "(function(){if(a){}g()})"
}];
(function testAllRecursiveFunctions() {
    for (var i = 0; i < recursiveFunctions.length; ++i) {
        var a = recursiveFunctions[i];
        eval(a.text.replace(/@/g, ""))
    }
})();
gcslice(2868);
Function("v={c:[{x:[[]],N:{x:[{}[d]]}}]}=minorgc(true)")()


crashes js debug shell (tested with a threadsafe deterministic 64-bit debug build) on m-c changeset c193fdeb4932 without any CLI arguments at Assertion failure: thing->isAligned(), at gc/Marking.cpp when the shell is compiled with --enable-gcgenerational --enable-exact-rooting.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack of 2nd assert

(function(){})
gc();
var recursiveFunctions = [{
    text: "(function(){if(a){}g()})"
}];
(function testAllRecursiveFunctions() {
    for (var i = 0; i < recursiveFunctions.length; ++i) {
        var a = recursiveFunctions[i];
        eval(a.text.replace(/@/g, ""))
    }
})();
gcslice(2868);
Function("v={c:[{x:[[]],N:{x:[{}[d]]}}]}=minorgc(true)")()

Assertion failure: MapTypeToTraceKind<T>::kind == GetGCThingTraceKind(thing), at gc/Marking.cpp when testcase is passed in as a CLI argument.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack of 3rd assert

function f(){}
gc();
var recursiveFunctions = [{
    text: "(function(){if(a){}g()})"
}];
(function testAllRecursiveFunctions() {
    for (var i = 0; i < recursiveFunctions.length; ++i) {
        var a = recursiveFunctions[i];
        eval(a.text.replace(/@/g, ""))
    }
})();
gcslice(2868);
Function("v={c:[{x:[[]],N:{x:[{}[d]]}}]}=minorgc(true)")()


Assertion failure: !InFreeList(thing->arenaHeader(), thing), at gc/Marking.cpp

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0

The problem exposed here is that the minor GC is happening between sweep slices. In this case, a JSFunction's LazyScript has been swept, but the JSFunction is background finalized, so hasn't been swept yet. We don't run the background finalizer until fully after the incremental GC, not between slices as I thought.

The obvious solution here is to only mark in the minor GC if we are not between sweep slices or the thing is marked. However, this make some other ggc tests fail. I did not do a full investigation, but we end up with a tenured array in the mark stack and one or more of its slots in the nursery. It should have been relocated, presumably, by the minorGC we run at the start of each GC slice. Apparently, we need to reify even dead objects.

Bill and I discussed several potential solutions, none of which are particularly awesome. I thought of one other solution a few seconds after Bill walked out the door though, which I think is somewhat more agreeable.

Bill, the exact assertion that is firing is:
    /*
     * Try to assert that the thing is allocated.  This is complicated by the
     * fact that allocated things may still contain the poison pattern if that
     * part has not been overwritten, and that the free span list head in the
     * ArenaHeader may not be synced with the real one in ArenaLists.
     */
    JS_ASSERT_IF(IsThingPoisoned(thing) && rt->isHeapBusy(),
                 !InFreeList(thing->arenaHeader(), thing));

This is the last statement in CheckMarkedThing and the last statement before we enter our custom mark function. Our minor GC marker will immediately exit without observing the object's broken contents because this is not a nursery thing. There are a handful of checks in CheckMarkedThing that may observe |thing|'s contents, but only as a side-effect.

Secondly, we only have to worry about edges from background finalized things to foreground finalized things. This is a very small set: I think it may actually be just this one edge.

Thus, I think the right solution here is to just skip these checks when in a minor GC and in a gc state that may have an incoherent heap.

Bill, to answer your question from earlier about how this test overflowed the store buffer in the first place: the test calls minorgc(true), which explicitly sets the overflowed bit on the store buffer.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 771716 [details] [diff] [review]
v0

Sorry, I don't think this is a good solution. It makes our invariants way more complex and it's likely to blow up in our faces later.

Here are the options:

1. Eliminate fallback marking and crash if we run out of dynamically-allocated store buffer space. This seems fine to me, particularly given that we're only in this situation because the test case explicitly asked for it.

2. I realized that we should be able to run background finalization before the minor GC without too much trouble. I think you should be able to do something like this:

    /* Set up list of zones for sweeping of background things. */
    JS_ASSERT(!rt->gcSweepingZones);
    for (GCZonesIter zone(rt); !zone.done(); zone.next()) {
        zone->gcNextGraphNode = rt->gcSweepingZones;
        rt->gcSweepingZones = zone;
    }

    /* Immediately sweep anything scheduled for background finalization. */
    SweepBackgroundThings(rt, false);

3. Figure out why checking the mark bits didn't work.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #4)
> Comment on attachment 771716 [details] [diff] [review]
> 
> 3. Figure out why checking the mark bits didn't work.

IIRC, checking the mark bits did work.

However, I'm convinced we should go the other route now in any case. Our perf problem on pdfjs is two-fold. First, we're blowing through store buffer space and constantly re-running the filter on already-filtered items. Simply removing one filter improves our perf from 48 to 4800. Secondly, we're blowing out the generic buffer and doing ~50 40-50ms fallback minor GCs.

Some observations:
(1) We're always going to run into a performance cliff of this magnitude if we don't switch to a growable store buffer.
(2) The algorithmic performance of the store buffer is more important than the bit-shuffling performance to a shocking degree.
(3) We can mitigate the OOM situation significantly by keeping a ballast and designing our data structure in such a way that incrementally growing does not need to reallocate the full buffer.

I discovered the pdfjs bits by accident when doing cleanup in preparation for rewriting the store buffer's storage. I'll clean those up and attach them separately since they're a clear win in either case and are somewhat prerequisite to fixing this bug for real.

Comment 6

[Terrence Cole [:terrence]]

Attachment: v0: Eagerly apply inRememberedSet.

- This checks inRememberedSet on put and removes compactNotInSet.

- This required moving some things into the header so that we can avoid going out-of-line for put.

- This also fixes a bug where we set about-to-overflow /before/ deduping. This leads us to do a minor GC way too early, which leads us to not growing the store buffer, which ends up totally destroying our performance on earley boyer.

- This also adds |explicit| to all of the internal classes.

- This also moves two assertions in CheckMarkedThing above the use of those pointers.

Sorry I omnibussed so much stuff into this patch. I didn't split it up as I went and tearing it all apart now would be a pain.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 773385 [details] [diff] [review]
v0: Eagerly apply inRememberedSet.

Looks like this doesn't work for RelocatablePtr, which only popped up with deeper testing. Will fix and re-upload.

Comment 8

[Terrence Cole [:terrence]]

Attachment: Part 0: eagerly check remembered set membership; v2

I started seeing other crashes after I rebased, but it was all bug 892287 and bug 891773. With those patches applied, this passes jit-tests --tbpl.

Comment 9

[Terrence Cole [:terrence]]

Attachment: Part 1: Implement a growable store buffer and rebase generic buffer; v0

This implements a growable store buffer and uses it to allocate backing storage for GenericBuffer. The new StoreBuffer::Storage class is a singly linked list of StoreBuffer::Storage::Block. The default block size corresponds to typical windows allocation granularity, but this is a totally arbitrary choice. Append is trivial and has O(1) behavior. Iteration of the buffer is encapsulated by StoreBuffer::Storage::Range. Because GenericBuffer does not store a single, static type and because MonoTypeBuffer needs to be able to update the buffer in-place, Range has a somewhat verbose interface. In practice, it seems to work well, however.

This passes all jsapi and jit tests with the block size in the patch and with a block size of 0x30, which heavily stresses the grow/truncate logic. It raises pdfjs perf from 48 to ~8000 and seems to stop the intermittent font-not-found errors there.

I've tried to keep the patches in this series minimal to ease reviewing. If you want me to split up the prerequisite cleanup/eagerization patch so you can get to this faster, just let me know.

Comment 10

[Terrence Cole [:terrence]]

Attachment: Part 2: Make MonoTypeBuffer growable.

This makes MonoTypeBuffer use StoreBuffer::Storage for its backing buffer. I haven't tested the performance yet, since there is still a bunch of code to rip out of Nursery::collect, but this seems to eliminate the intermittent timeouts I get on GGC builds on Environment-find-06.

Comment 11

[Terrence Cole [:terrence]]

Attachment: Part 3: Fix the minorgc testing function

Now that there is not a fallback GC, the bool parameter to minorgc does setAboutToOverflow instead of setOverflowed.

Comment 12

[Terrence Cole [:terrence]]

Attachment: Part 4: Add tests; v0

This adds the tests from this bug, which now pass on top of this stack.

Comment 13

[Terrence Cole [:terrence]]

Attachment: Part 5: remove overflow state; v0

The first round of cleanup:

 7 files changed, 14 insertions(+), 117 deletions(-)

Comment 14

[Terrence Cole [:terrence]]

Attachment: Part 6: Use a single EdgeSet instead of six; v0

We don't use this table concurrently, so share it.

Comment 15

[Terrence Cole [:terrence]]

Before
======
Richards: 13372
DeltaBlue: 15478
Crypto: 15492
RayTrace: 28757
EarleyBoyer: 11779
RegExp: 2079
Splay: 4961
NavierStokes: 20459
PdfJS: fail
Mandreel: 4062
Gameboy: 17946
CodeLoad: 14823
Box2D: 5311
----
Score (version 8): 10231 (w/out pdfjs)

After
=====
Richards: 13581
DeltaBlue: 15465
Crypto: 16738
RayTrace: 25604
EarleyBoyer: 11199
RegExp: 2367
Splay: 4645
NavierStokes: 20182
PdfJS: 7707
Mandreel: 4056
Gameboy: 21032
CodeLoad: 14625
Box2D: 5306
----
Score (version 8): 10102

d8
==
Richards: 13567
DeltaBlue: 17330
Crypto: 16223
RayTrace: 16132
EarleyBoyer: 26226
RegExp: 3898
Splay: 5289
NavierStokes: 17885
PdfJS: 13018
Mandreel: 13986
Gameboy: 13636
CodeLoad: 14719
Box2D: 13116
----
Score (version 8): 12940

Tip
===
Richards: 14901
DeltaBlue: 12994
Crypto: 16577
RayTrace: 15730
EarleyBoyer: 14742
RegExp: 2695
Splay: 12427
NavierStokes: 20270
PdfJS: 7972
Mandreel: 4130
Gameboy: 21557
CodeLoad: 15291
Box2D: 5591
----
Score (version 8): 10900


The main takeaway is that with pdfjs passing, this should get us into the right ballpark. Other than that, there is a small regression across the board and a big regression on Raytrace. This is a laptop, however, and my numbers have lots of noise. I'll look at the generated code next to ensure that clang isn't doing something boneheaded for the new |put| implementation.

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 776527 [details] [diff] [review]
Part 5: remove overflow state; v0

At lunch David convinced me that we should keep this available as an option.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 775046 [details] [diff] [review]
Part 0: eagerly check remembered set membership; v2

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

::: js/src/gc/StoreBuffer.h
@@ +149,2 @@
>                  if (isFull()) {
>                      compact();

I don't think this extra compact() is going to do any good now that you have one above it.

@@ +316,5 @@
>          bool operator!=(const SlotEdge &other) const {
>              return object != other.object || offset != other.offset || kind != other.kind;
>          }
>  
> +        HeapSlot *slotLocation() const;

I'm kinda worried that this isn't inline. Can you see if it affects performance?

@@ +407,5 @@
>                                      RelocValueBufferSize + RelocCellBufferSize +
>                                      GenericBufferSize;
>  
>    public:
> +    explicit StoreBuffer(JSRuntime *rt, const Nursery &n)

Can you call it nursery instead of n?

Comment 18

[Terrence Cole [:terrence]]

Attachment: Part 5: rework overflow mechanics; v1

This passes the tests in this bug with the new if-statement in the heap tracing.

Comment 19

[Terrence Cole [:terrence]]

Attachment: Part 7: Rename aboutToOverflow to needsGC; v0

This better matches the new purpose and use of this flag.

Comment 20

[Terrence Cole [:terrence]]

Attachment: Part 0.1: inline the in-remembered-set computation for slots; v0

You were right about the uninlining: it gives ~100pt drop. Inlining the inRememberedSet computation is a bit painful for slots, but it does get us back what we lost and ~50-100pts more, probably because of the isObject checks.

This is a diff on top of Part0, after re-inlined SlotEdge.

Comment 21

[Terrence Cole [:terrence]]

Attachment: Part 8 - replace StoreBuffer::Storage with LifoAlloc; v0

Replace our custom allocator with LifoAlloc. This only saves ~50 lines, but LifoAlloc already exists, is well tested, and contains a couple of nice-to-have features when I didn't implement for Storage.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776479 [details] [diff] [review]
Part 1: Implement a growable store buffer and rebase generic buffer; v0

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

::: js/src/gc/StoreBuffer.h
@@ +356,5 @@
>  
>          GenericBuffer &operator=(const GenericBuffer& other) MOZ_DELETE;
>  
> +        void enable() { enabled_ = true; }
> +        void disable() { enabled_ = false; }

You might want to clear when you disable.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776696 [details] [diff] [review]
Part 7: Rename aboutToOverflow to needsGC; v0

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

I'd be okay renaming this to needsMinorGC, but needsGC is confusing.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776696 [details] [diff] [review]
Part 7: Rename aboutToOverflow to needsGC; v0

Actually, I changed my mind. I think the current name is more descriptive than the others.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776771 [details] [diff] [review]
Part 0.1: inline the in-remembered-set computation for slots; v0

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

::: js/src/gc/StoreBuffer.h
@@ +427,5 @@
>          if (!edge.inRememberedSet(nursery))
>              return;
>          bufferCell.put(edge);
>      }
> +    void putSlot(JSObject *obj, HeapSlot::Kind kind, uint32_t slot, void *target) {

I think putSlot should take the target as a Value. Then it could do the isObject test itself. Then you could remove all the other isObject checks.

::: js/src/jsobj.cpp
@@ +2080,5 @@
>           * Trigger post barriers for fixed slots. JSObject bits are barriered
>           * below, in common with the other case.
>           */
>          for (size_t i = 0; i < a->numFixedSlots(); ++i) {
> +            HeapSlot::writeBarrierPost(cx->runtime(), a, HeapSlot::Slot, i, &a->getSlotRef(i));

This isn't the right target. But if you change the target to a Value, you can just pass a->getSlot(i) here.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm getting really confused trying to review this stuff. Can you fold parts 1, 2, 5, and 8 into a single patch? Then I'll review that.

Comment 28

[Terrence Cole [:terrence]]

Attachment: Part 1, 2, 5, and 8: rollup

Yeah, I probably should have just done this rollup when switching to LifoAlloc.

Comment 29

[Terrence Cole [:terrence]]

Attachment: Part 0.1 - Inline inRememberedSet for putSlot - v1

Thanks Bill, that's much nicer. I'm not sure what I was thinking here.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 779456 [details] [diff] [review]
Part 0.1 - Inline inRememberedSet for putSlot - v1

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

::: js/src/gc/Barrier-inl.h
@@ +369,4 @@
>  {
>  #ifdef JSGC_GENERATIONAL
> +    if (target.isObject())
> +        obj->runtime()->gcStoreBuffer.putSlot(obj, kind, slot, &target.toObject());

Can you just call writeBarrierPost(obj->runtime(), obj, kind, slot, target) ?

@@ +378,3 @@
>  {
>  #ifdef JSGC_GENERATIONAL
> +    if (target.isObject())

It would be great to assert the following:

JS_ASSERT_IF(kind == Slot, obj->getSlot(slot) == target);
JS_ASSERT_IF(kind == Element, obj->getDenseElement(slot) == target);

I think that should work.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 779416 [details] [diff] [review]
Part 1, 2, 5, and 8: rollup

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

I'm confused about this patch. It makes the store buffer growable, but I don't see where we ever actually grow it.

::: js/src/ds/LifoAlloc.h
@@ +87,5 @@
>      void setNext(BumpChunk *succ) { next_ = succ; }
>  
>      size_t used() const { return bump - bumpBase(); }
>  
> +    void *start() const { return AlignPtr(bumpBase()); }

I think bumpBase() should already be aligned. Can you just add an assert that it is, and get rid of the AlignPtr here?

@@ +349,5 @@
>      }
>  
> +    // Return true if the LifoAlloc does not currently contain any allocations.
> +    bool isEmpty() const {
> +        return !latest;

I think you want !latest || !latest->used().

@@ +418,5 @@
> +            position_(static_cast<char *>(alloc.first ? alloc.first->start() : NULL))
> +        {}
> +
> +        // Return true if there are no more bytes to enumerate.
> +        bool empty() { return !chunk_ || position_ >= alloc_->latest->mark(); }

This doesn't seem right. position_ is within chunk_, which could be located after alloc_->latest in memory. I think you want this:

!chunk || (chunk_ == alloc_->latest && position_ >= chunk_->mark())

@@ +436,5 @@
> +        // Update the bytes at the current position with a new value.
> +        template <typename T>
> +        void updateFront(const T &t) {
> +            ensureSpaceAndAlignment(sizeof(T));
> +            memmove(reinterpret_cast<void *>(position_), &const_cast<T &>(t), sizeof(T));

Are either of these casts needed? memmove takes a const void * for its second param.

::: js/src/gc/Nursery.cpp
@@ +530,5 @@
> +                 * During sweeping, we have to be careful not to touch objects
> +                 * which are not live: they may have already been swept.
> +                 */
> +                if (trc->runtime->gcIncrementalState == SWEEP &&
> +                    (!cell->isMarked() && !cell->arenaHeader()->allocatedDuringIncremental))

I thought we weren't doing this? Isn't that the whole point of the patch?

::: js/src/gc/StoreBuffer.h
@@ +109,5 @@
>          MonoTypeBuffer &operator=(const MonoTypeBuffer& other) MOZ_DELETE;
>  
> +        void enable() { enabled_ = true; }
> +        void disable() { enabled_ = false; clear(); }
> +        void clear() { storage_.used() ? storage_.releaseAll() : storage_.freeAll(); }

What's the rationale for this?

@@ +114,3 @@
>  
> +        bool isAboutToOverflow() const {
> +            return !storage_.isEmpty() && storage_.availableInCurrentChunk() < MinAvailableSize;

This test has a bad property: it could be that isAboutToOverflow() is true, but then you do some put() operations and it becomes false. I think we should add an sizeIsAbove(n) method to LifoAlloc. Then we could test here if storage_.sizeIsAbove(x), where x is (1<<16)-MinAvailableSize or something.

@@ +134,3 @@
>                  return;
>  
> +            T *tp = storage_.newPod<T>();

Why aren't you just using new_?

@@ +223,2 @@
>  
> +            T *tp = storage_.newPod<T>();

Again, it seems like you could use new_ here.

Comment 32

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #31)
> Comment on attachment 779416 [details] [diff] [review]
> Part 1, 2, 5, and 8: rollup

Thanks for the thorough review!

> I'm confused about this patch. It makes the store buffer growable, but I
> don't see where we ever actually grow it.

It happens implicitly: |new_| calls |alloc| calls |getOrCreateChunk|.

> ::: js/src/ds/LifoAlloc.h
> @@ +87,5 @@
> >      void setNext(BumpChunk *succ) { next_ = succ; }
> >  
> >      size_t used() const { return bump - bumpBase(); }
> >  
> > +    void *start() const { return AlignPtr(bumpBase()); }
> 
> I think bumpBase() should already be aligned. Can you just add an assert
> that it is, and get rid of the AlignPtr here?

Good point. I was confused because I did not see an assertion that bumpBase is aligned, so I did not want to assume that it was. After looking harder, however, I think that the constuctor's assertion of |bump == AlignPtr(bump)| does already actually assert this property indirectly, so I'm just going to drop the AlignPtr here.
 
> @@ +349,5 @@
> >      }
> >  
> > +    // Return true if the LifoAlloc does not currently contain any allocations.
> > +    bool isEmpty() const {
> > +        return !latest;
> 
> I think you want !latest || !latest->used().

Good catch!

> @@ +418,5 @@
> > +            position_(static_cast<char *>(alloc.first ? alloc.first->start() : NULL))
> > +        {}
> > +
> > +        // Return true if there are no more bytes to enumerate.
> > +        bool empty() { return !chunk_ || position_ >= alloc_->latest->mark(); }
> 
> This doesn't seem right. position_ is within chunk_, which could be located
> after alloc_->latest in memory. I think you want this:
>
> !chunk || (chunk_ == alloc_->latest && position_ >= chunk_->mark())

Good catch, again.
 
> @@ +436,5 @@
> > +        // Update the bytes at the current position with a new value.
> > +        template <typename T>
> > +        void updateFront(const T &t) {
> > +            ensureSpaceAndAlignment(sizeof(T));
> > +            memmove(reinterpret_cast<void *>(position_), &const_cast<T &>(t), sizeof(T));
> 
> Are either of these casts needed? memmove takes a const void * for its
> second param.

Huh, seems not.

> ::: js/src/gc/Nursery.cpp
> @@ +530,5 @@
> > +                 * During sweeping, we have to be careful not to touch objects
> > +                 * which are not live: they may have already been swept.
> > +                 */
> > +                if (trc->runtime->gcIncrementalState == SWEEP &&
> > +                    (!cell->isMarked() && !cell->arenaHeader()->allocatedDuringIncremental))
> 
> I thought we weren't doing this? Isn't that the whole point of the patch?

Well, there are a couple intersecting concerns here. There are two ways to solve the specific crash in this bug: the above hunk, or the growable store buffer that makes up the rest of the patch. Since the growable store buffer also fixes our performance cliff -- manifesting heavily on pdf.js right now -- we want this for sure. Since you asked why the above did not work in your initial comment on the new code, it was not clear to me that you wanted to completely remove the fallback marker after all.

Thinking about it some more, I think it would probably be possible to design a client script that could allocate the full heap, then make arbitrarily many store buffer entries, exposing users to a DOS. Not the end of the world, but I think it is probably worth keeping the fallback marking around, since it isn't much code and we have a couple of weeks of solid fuzzing against it now.

> ::: js/src/gc/StoreBuffer.h
> @@ +109,5 @@
> >          MonoTypeBuffer &operator=(const MonoTypeBuffer& other) MOZ_DELETE;
> >  
> > +        void enable() { enabled_ = true; }
> > +        void disable() { enabled_ = false; clear(); }
> > +        void clear() { storage_.used() ? storage_.releaseAll() : storage_.freeAll(); }
> 
> What's the rationale for this?

If we have not used this store buffer for awhile (since the last minor GC), then return all of our memory -- including the first block -- and lazily re-acquire if we use it again. On the other hand, if we are using this store buffer actively, keep any memory that we've allocated as we're likely to need it again soon.

I considered adding a function like freeAllButFirstBlock, but I wasn't sure if it would be worth the extra code.
 
> @@ +114,3 @@
> >  
> > +        bool isAboutToOverflow() const {
> > +            return !storage_.isEmpty() && storage_.availableInCurrentChunk() < MinAvailableSize;
> 
> This test has a bad property: it could be that isAboutToOverflow() is true,
> but then you do some put() operations and it becomes false. I think we
> should add an sizeIsAbove(n) method to LifoAlloc. Then we could test here if
> storage_.sizeIsAbove(x), where x is (1<<16)-MinAvailableSize or something.

Actually, I don't think this is a bad property. The purpose of triggering a minor GC here is to keep us from needing to allocate another chunk (and risking not getting one). If the situation above does happen, we've already successfully allocated the memory, so we should now put off GCing until the nursery fills, if possible.

> @@ +134,3 @@
> >                  return;
> >  
> > +            T *tp = storage_.newPod<T>();
> 
> Why aren't you just using new_?

I managed to not see the JS_DECLARE_NEW_METHODS despite having searched for it and despite it being two lines above my changes. I must have typoed while searching for it or something.

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Thinking about it some more, I think it would probably be possible to design a client script
> that could allocate the full heap, then make arbitrarily many store buffer entries, exposing
> users to a DOS. Not the end of the world, but I think it is probably worth keeping the
> fallback marking around, since it isn't much code and we have a couple of weeks of solid
> fuzzing against it now.

There are already plenty of ways to DOS the browser in precisely this way. For example, this patch aborts if it can't allocate a big enough hash table for the relocatable store buffer. I'd rather we have one more DOS patch if we can remove the tracing code. Despite the fuzzing it's gotten, it still may have bugs that we don't know about, and it adds complexity.

> Actually, I don't think this is a bad property. The purpose of triggering a minor GC here is
> to keep us from needing to allocate another chunk (and risking not getting one). If the
> situation above does happen, we've already successfully allocated the memory, so we should now
> put off GCing until the nursery fills, if possible.

Looking at this again, it looks like we call setAboutToOverflow on the StoreBuffer the first time isAboutToOverflow goes to true, so we're going to do a minor GC no matter what. So I guess it doesn't matter what we do here. However, I still think it would be simpler if it worked the way I described. Also, what's the purpose of the isEmpty() check?

Comment 34

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #33)
> > Thinking about it some more, I think it would probably be possible to design a client script
> > that could allocate the full heap, then make arbitrarily many store buffer entries, exposing
> > users to a DOS. Not the end of the world, but I think it is probably worth keeping the
> > fallback marking around, since it isn't much code and we have a couple of weeks of solid
> > fuzzing against it now.
> 
> There are already plenty of ways to DOS the browser in precisely this way.
> For example, this patch aborts if it can't allocate a big enough hash table
> for the relocatable store buffer. I'd rather we have one more DOS patch if
> we can remove the tracing code. Despite the fuzzing it's gotten, it still
> may have bugs that we don't know about, and it adds complexity.

Fair enough. I'll rip it all out.

> > Actually, I don't think this is a bad property. The purpose of triggering a minor GC here is
> > to keep us from needing to allocate another chunk (and risking not getting one). If the
> > situation above does happen, we've already successfully allocated the memory, so we should now
> > put off GCing until the nursery fills, if possible.
> 
> Looking at this again, it looks like we call setAboutToOverflow on the
> StoreBuffer the first time isAboutToOverflow goes to true, so we're going to
> do a minor GC no matter what. So I guess it doesn't matter what we do here.
> However, I still think it would be simpler if it worked the way I described.

Great point. Now that this is more paged in, the reason I didn't implement sizeIsAbove before is that LifoAlloc does not track the total size. We would need to walk the chunk list to count the size or add a new field and appropriate machinery. An O(n) list traversal per store buffer insertion could potentially be very bad, but an add in that path would be somewhat bad all the time. Rather than benchmark I went with the more-complicated-but-trivially-performance-neutral solution that currently exists.

> Also, what's the purpose of the isEmpty() check?

If we haven't allocated into the LifoAlloc yet, the latest chunk will be NULL and availableInCurrentChunk will return 0. This seemed to be better than returning our default chunk size or a sentinel, as the LifoAlloc is free to make a bigger allocation. I could also easily see this method being useful for "can we fit this without allocation?" tests in other contexts.

Comment 35

[Terrence Cole [:terrence]]

Comment on attachment 779456 [details] [diff] [review]
Part 0.1 - Inline inRememberedSet for putSlot - v1

Part 0 took us from 10000 to 15000 on octane, time to see what the rest of it will do.

Comment 36

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0e01422cb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb89f19070ae

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a0e01422cb3
https://hg.mozilla.org/mozilla-central/rev/eb89f19070ae