Get it working in Holly and verify that it works. Ideally, we can get the verifier running on central, but that depends on us getting the store buffer there first, is going to be a bit more challenging. In the meantime, getting this working again on Holly will be extremely useful.

Copied the StoreBuffer from holly to inbound, re-implemented on top of inbound to ensure that it's not missing anything in holly, then ported back to holly to ensure that it actually works. This fails on inbound (with empty post barriers) and works on holly (with post barriers).

https://tbpl.mozilla.org/?tree=Try&rev=bb8c403c266d I think this is ready for review.

Comment on attachment 635545 [details] [diff] [review] v1: Cleaned up and added comments and fixed some missing ifdefs. Review of attachment 635545 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. My main concerns: 1. There's a lot of dead code in here, or code that's not really used. I'm in favor of checking stuff in early, but not this much; people will just get confused. I think that StoreBuffer.cpp could be significantly shortened by removing all the page manipulation, the high/low water marks, the marking, and compaction code except for the unput handling. I don't think any of this is needed for verification. 2. The locationOf and deref stuff is incredibly confusing. Please replace with more specific functionality. For example, in several cases when you call deref, you're really asking "is this thing a pointer to a GC cell?". 3. I'd like there to be more freedom about what ends up in the nursery set. It would be great if you could take a random seed and use that to decide which newly allocated objects should go in the nursery. 4. The scheduling of verification needs work. Let's talk about this at some point. ::: js/src/gc/Memory.h @@ +30,5 @@ > // and should be paged in and out normally. This may be a no-op on some > // platforms. > bool MarkPagesInUse(void *p, size_t size); > > +// Cause a page fault for writes to the given page aligned area. It seems more appropriate to say "Cause a crash ...". ::: js/src/gc/StoreBuffer.cpp @@ +96,5 @@ > +/*** EdgeBuffer ***/ > + > +template <class T> > +void > +StoreBuffer::EdgeBuffer<T>::init(StoreBuffer *owner_, Nursery *nursery_, bool mayHaveRemovals_) None of this stuff can fail. Can it go in a constructor instead? @@ +146,5 @@ > + if (!MarkPagesReadOnly(region + len - PageSize, PageSize)) > + return false; > + > + /* TODO: profile to find out if this a good idea. */ > + MarkPagesUnused(region + PageSize, len - PageSize); In the first implementation, I'd rather leave off all this stuff with the low and high water marks, the read-only pages, and the the unused pages. @@ +350,5 @@ > + JS_ASSERT(mayHaveRemovals); > + T *p = pos; > + put(v); > + uintptr_t tmp = (uintptr_t)*p; > + *p = (T)(tmp | 1); Please add a Tag() function and then just do put(Tag(v)). @@ +368,5 @@ > +{ > + compactFully(); > + T *cursor = base; > + while (cursor != pos) { > + /* Remove moved must have removed all tagged pointers. */ This is a confusing comment. Do you mean compaction should have removed all tagged pointers? @@ +381,5 @@ > + > +/*** GenericBuffer ***/ > + > +void > +StoreBuffer::GenericBuffer::init(StoreBuffer *owner_, Nursery *nursery_) Same here about the constructor. @@ +396,5 @@ > + if (!MarkPagesReadOnly(region + len - PageSize, PageSize)) > + return false; > + > + /* TODO: profile to find out if this is this a good idea. */ > + MarkPagesUnused(region + PageSize, len - PageSize); Please remove this page stuff. We don't need it now. @@ +464,5 @@ > +StoreBuffer::init(Nursery *nursery_) > +{ > + nursery = nursery_; > + enabled = false; > + edgeSet.init(); This can fail. @@ +477,5 @@ > + > +bool > +StoreBuffer::enable() > +{ > + buffer = MapAlignedPages(TotalSize, Alignment); I can't remember. Why does this memory have to be aligned? ::: js/src/gc/StoreBuffer.h @@ +26,5 @@ > +{ > + HashSet<void*, PointerHasher<void*, 3>, SystemAllocPolicy> nursery; > + > + public: > + Nursery() : nursery() {} Empty line after this. @@ +49,5 @@ > + } > +}; > + > +/* > + * BufferableRef represents an abstract reference for use in the Generational lowercase G in generational @@ +76,5 @@ > + public: > + HashKeyRef(Map *m, const Key &k) : map(m), key(k) {} > + > + bool match(void *location) { > + typename Map::Ptr p = map->lookup(key); I think it's typical to make a private typedef for Map::Ptr in HashKeyRef's body. @@ +90,5 @@ > + Key key(p->key); > + Mark(trc, &key, "HashKeyRef"); > + if (key != p->key) { > + map->put(key, p->value); > + map->remove(p); Don't we have to worry about memory allocation here (i.e., if put fails)? Would it be okay to use rekey instead? @@ +114,5 @@ > + > + /* > + * Since all of our other types are pointer types, we need to have > + * transparent pointer compatibility for typing, with the caveat that they > + * will never be used in practice. I don't understand this. Why are these needed, since you already provide specializations for SlotRef in several places? @@ +125,5 @@ > + JS_NOT_REACHED("SlotRef void* constructor"); > + } > +}; > + > +class Nursery; No need for a forward declaration since it's declared above. @@ +129,5 @@ > +class Nursery; > + > +/* > + * The write buffer is responsible for tracking all writes that update an > + * object (excluding writes that are not-provably-not-in the remembered set). This comment could be a lot clearer. @@ +133,5 @@ > + * object (excluding writes that are not-provably-not-in the remembered set). > + */ > +class StoreBuffer > +{ > + /* Note: Match window's mapping granularity. */ Do you mean Windows's? @@ +134,5 @@ > + */ > +class StoreBuffer > +{ > + /* Note: Match window's mapping granularity. */ > + static const size_t Alignment = 64 * 1024; I would call this BaseSize, or something like that. @@ +157,5 @@ > + * Write buffers which are representable as a simple > + * pointer-to-a-reference use this buffer type. > + */ > + template<typename T> > + class EdgeBuffer You're putting SlotRefs in here, and those aren't simple pointers. I think a better name for this would be FixedSizeEntryBuffer. @@ +249,5 @@ > + size_t count() const; > + > + template <class T> > + void put(const T &t) { > + JS_STATIC_ASSERT((sizeof(T) + sizeof(unsigned)) % sizeof(int) == 0); What's this for? @@ +268,5 @@ > + EdgeBuffer<Cell **> bufferCell; > + EdgeBuffer<SlotRef> bufferSlot; > + EdgeBuffer<Value *> bufferVal; > + EdgeBuffer<Cell **> bufferRelocCell; > + EdgeBuffer<Value *> bufferRelocVal; Need a comment somewhere that the Reloc ones are the ones where elements can be removed before GC happens. @@ +300,5 @@ > + if (!enabled) > + return; > + bufferCell.put(o); > + } > + void putSlot(JSObject *obj, uint32_t slotno) { |slotno| -> |slot|, please. ::: js/src/jscompartment.cpp @@ +43,5 @@ > + arenas(), > +#ifdef JSGC_GENERATIONAL > + gcStoreBuffer(), > + gcNursery(), > +#endif Won't this happen automatically? @@ +92,5 @@ > return false; > > +#ifdef JSGC_GENERATIONAL > + gcNursery.init(); > + gcStoreBuffer.init(&gcNursery); You need to check for failure here. ::: js/src/jsgc.cpp @@ +3100,5 @@ > MarkRuntime(gcmarker); > } > > +static bool > +MarkWeakIteratively(JSTracer *trc) What's the reason for factoring this out? @@ +4279,5 @@ > > /* > * Write barrier verification > * > + * The next few functions are for incremental write barrier verification. Remove the word "incremental", since this function is now called for incremental and generational verification. @@ +4625,3 @@ > { > +#ifdef JSGC_GENERATIONAL > + if (rt->gcVerifyData) We probably don't want to do this if we're in the middle of an incremental GC, either. @@ +4641,5 @@ > +PostVerifierVisitEdge(JSTracer *jstrc, void **thingp, JSGCTraceKind kind) > +{ > + VerifyTracer *trc = (VerifyTracer *)jstrc; > + Cell *src = trc->src; > + Cell *tgt = (Cell *)*thingp; |dst| is more standard than |tgt|. @@ +4666,5 @@ > + * 2) Step until it becomes == <value of tgt> > + * 3) Figure out why this assignment is not inserting loc into into the > + * store buffer. > + */ > + JS_ASSERT(comp->gcStoreBuffer.containsEdgeAt(loc)); Please see how the pre-barrier verifier asserts now. It prints friendlier messages so that the fuzzers can sort out different versions of the assertion. @@ +4687,5 @@ > + > + VerifyTracer *trc = (VerifyTracer *)rt->gcVerifyData; > + JS_TracerInit(trc, rt, PostVerifierVisitEdge); > + rt->gcNumber++; > + trc->number = rt->gcNumber; You probably want to increment the gcNumber and save it in the beginning of the verification. @@ +4692,5 @@ > + trc->count = 0; > + > + PurgeRuntime(trc); > + > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { Please use |c|, rather than |comps|, since that's what we always do. @@ +4693,5 @@ > + > + PurgeRuntime(trc); > + > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > + JSCompartment *comp = comps.get(); No need for this. Just use c->field below. @@ +4695,5 @@ > + > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > + JSCompartment *comp = comps.get(); > + comp->gcStoreBuffer.coalesceForVerification(); > + comp->setNeedsBarrier(false); What's the setNeedsBarrier for? @@ +4698,5 @@ > + comp->gcStoreBuffer.coalesceForVerification(); > + comp->setNeedsBarrier(false); > + } > + > + /* Walk the heap. Note: we MarkRuntime in GCNursery, so don't visit it. */ What does this mean? @@ +4700,5 @@ > + } > + > + /* Walk the heap. Note: we MarkRuntime in GCNursery, so don't visit it. */ > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > + JSCompartment *comp = comps.get(); Same deal here as above. @@ +4703,5 @@ > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > + JSCompartment *comp = comps.get(); > + if (comp->watchpointMap) > + comp->watchpointMap->markAll(trc); > + comp->markCrossCompartmentWrappers(trc); Why do you need to markCrossCompartmentWrappers? It expects only to be called during a real GC. @@ +4712,5 @@ > + for (CellIterUnderGC cells(comp, AllocKind(kind)); !cells.done(); cells.next()) { > + Cell *cell = cells.getCell(); > + trc->src = cell; > + > + JS_TraceChildren(static_cast<JSTracer *>(trc), (void *)cell, No need for either cast. VerifyTracer is a subclass of JSTracer. And Cell* coerces to void* automatically. @@ +4718,5 @@ > + > + if (AllocKind(kind) == FINALIZE_SCRIPT) { > + JSScript *script = cells.get<JSScript>(); > + if (script->hasScriptCounts) > + MarkScriptRoot(trc, &script, "profilingScripts"); This code is only relevant when profiling, and we'll never be running the verifier when profiling, so it can be removed. @@ +4744,5 @@ > +{ > + if (rt->gcZeal() == ZealVerifierValue) > + StartVerifyPreBarriers(rt); > + else if (rt->gcZeal() == ZealVerifierPostValue) > + StartVerifyPostBarriers(rt); This won't really work. First, it doesn't support the Frame modes. Second, we sometimes call these functions directly from the shell without any GC zeal being set (via verifybarriers()). I think it would be better just to kill off the generic Start/EndVerifyBarriers. We'll need to specialize VerifyBarriers into VerifyPreBarriers and VerifyPostBarriers (or maybe pass it an argument to specify which one. Then we'll call those based on the GC zeal mode in NotifyDidPaint. We'll need to split the verifybarriers() shell call into two versions as well. MaybeVerifyBarriers is always based on GC zeal, so that one can pick the verifier based on the zeal mode. The calls to Start/EndVerifyBarriers in the Collect code will have to be based on which kind of verifier was already running when the GC happened. Can we stick that in rt->gcVerifierData?

(In reply to Bill McCloskey (:billm) from comment #3) > 1. There's a lot of dead code in here, or code that's not really used. I'm > in favor of checking stuff in early, but not this much; people will just get > confused. I think that StoreBuffer.cpp could be significantly shortened by > removing all the page manipulation, the high/low water marks, the marking, > and compaction code except for the unput handling. I don't think any of this > is needed for verification. I think it would it make sense to get this running on TBPL next to the root analysis. Maybe it would make sense to get the pre verifier up there as well? > 2. The locationOf and deref stuff is incredibly confusing. Please replace > with more specific functionality. For example, in several cases when you > call deref, you're really asking "is this thing a pointer to a GC cell?". Sorry for being so dense about understanding what you meant here and thanks for taking the time to break out the puppets and explain it to me IRL. It's always a bit shocking to me when I realize how very different my internal model of a problem is from everyone else's. I'm now no longer actively opposed to this approach: it's actually quite a bit nicer from an OO perspective. I think we can make this even nicer still and address my two chief complaints, but it's probably not worth the time right now. For later, we should: (1) remove the code duplication between the edge classes and (2) make it so we can easily use the same formulation of |inRememberedSet(Nursery)| between the verifier and the store buffer. > 3. I'd like there to be more freedom about what ends up in the nursery set. > It would be great if you could take a random seed and use that to decide > which newly allocated objects should go in the nursery. I was thinking we would do this in another bug. It should be easy enough to add on top of this work and I still need to port over the existing manual barriers before we can pass enough tests to give it to the fuzzers. > 4. The scheduling of verification needs work. Let's talk about this at some > point. I think I've managed to make the two completely independent. Ideally, the fuzzers won't have to worry about the state getting interleaved incorrectly when reducing. > ::: js/src/gc/Memory.h > @@ +30,5 @@ > > // and should be paged in and out normally. This may be a no-op on some > > // platforms. > > bool MarkPagesInUse(void *p, size_t size); > > > > +// Cause a page fault for writes to the given page aligned area. > > It seems more appropriate to say "Cause a crash ...". :-) Removed, with an ominous "for now." > ::: js/src/gc/StoreBuffer.cpp > @@ +96,5 @@ > > +/*** EdgeBuffer ***/ > > + > > +template <class T> > > +void > > +StoreBuffer::EdgeBuffer<T>::init(StoreBuffer *owner_, Nursery *nursery_, bool mayHaveRemovals_) > > None of this stuff can fail. Can it go in a constructor instead? I got lazy and didn't really revisit the basic construction process when I moved the bulk of the code into enable/disable. Things should be much nicer now. > @@ +146,5 @@ > > + if (!MarkPagesReadOnly(region + len - PageSize, PageSize)) > > + return false; > > + > > + /* TODO: profile to find out if this a good idea. */ > > + MarkPagesUnused(region + PageSize, len - PageSize); > > In the first implementation, I'd rather leave off all this stuff with the > low and high water marks, the read-only pages, and the the unused pages. Done. I'm going to let it stay in holly for the moment, but you are definitely right about this. > @@ +350,5 @@ > > + JS_ASSERT(mayHaveRemovals); > > + T *p = pos; > > + put(v); > > + uintptr_t tmp = (uintptr_t)*p; > > + *p = (T)(tmp | 1); > > Please add a Tag() function and then just do put(Tag(v)). I've attached the tagging directly to the edge classes. > @@ +368,5 @@ > > +{ > > + compactFully(); > > + T *cursor = base; > > + while (cursor != pos) { > > + /* Remove moved must have removed all tagged pointers. */ > > This is a confusing comment. Do you mean compaction should have removed all > tagged pointers? Should have spelled that as |compactRemoveMoved()|. This assertion was already present in that method in an identical form, so I've just removed it here. > @@ +477,5 @@ > > + > > +bool > > +StoreBuffer::enable() > > +{ > > + buffer = MapAlignedPages(TotalSize, Alignment); > > I can't remember. Why does this memory have to be aligned? This was so that the guard page at the end would be page aligned. I've replaced these allocations with js_malloc/js_free. > @@ +90,5 @@ > > + Key key(p->key); > > + Mark(trc, &key, "HashKeyRef"); > > + if (key != p->key) { > > + map->put(key, p->value); > > + map->remove(p); > > Don't we have to worry about memory allocation here (i.e., if put fails)? > Would it be okay to use rekey instead? Thank you for the reminder. Currently, we only have rekey on Enum, not on an arbitrary key yet. This is part of the marking path, which shouldn't be present anyway, so I've just removed it and will fix this up on holly. > @@ +114,5 @@ > > + > > + /* > > + * Since all of our other types are pointer types, we need to have > > + * transparent pointer compatibility for typing, with the caveat that they > > + * will never be used in practice. > > I don't understand this. Why are these needed, since you already provide > specializations for SlotRef in several places? IIRC, this is because we needed to be able to cast EdgeBuffer's T to a uintptr_t. It's probably a bad sign (or at least a sign of my C background) that I was needing so much handholding to make C++ typecheck. > @@ +129,5 @@ > > +class Nursery; > > + > > +/* > > + * The write buffer is responsible for tracking all writes that update an > > + * object (excluding writes that are not-provably-not-in the remembered set). > > This comment could be a lot clearer. Rewritten, but no guarantees: it's still my brain doing the writing after all. > @@ +133,5 @@ > > + * object (excluding writes that are not-provably-not-in the remembered set). > > + */ > > +class StoreBuffer > > +{ > > + /* Note: Match window's mapping granularity. */ > > Do you mean Windows's? Yes. There is a static var with size that Igor added to gc/Memory.cpp that should be exported in some more-sane way. > @@ +134,5 @@ > > + */ > > +class StoreBuffer > > +{ > > + /* Note: Match window's mapping granularity. */ > > + static const size_t Alignment = 64 * 1024; > > I would call this BaseSize, or something like that. I've made these proportional to the desired maximum number of elements in the buffer. > @@ +157,5 @@ > > + * Write buffers which are representable as a simple > > + * pointer-to-a-reference use this buffer type. > > + */ > > + template<typename T> > > + class EdgeBuffer > > You're putting SlotRefs in here, and those aren't simple pointers. I think a > better name for this would be FixedSizeEntryBuffer. There is a mild English ambiguity there between which of Entry and Buffer is fixed size. I've used MonoTypeBuffer for now, but it's quite easy to change if you prefer the other. > @@ +249,5 @@ > > + size_t count() const; > > + > > + template <class T> > > + void put(const T &t) { > > + JS_STATIC_ASSERT((sizeof(T) + sizeof(unsigned)) % sizeof(int) == 0); > That was to ensure that we do not run out of space in the middle of putting any single T. That micro-optimization is just wrong, however, as it would need to consider any combination of T's. Moreover, I can't find the place where I was using actually that assertion to (incorrectly) check for overflow anyway. I've made it just a trivial overflow check now. > @@ +268,5 @@ > > + EdgeBuffer<Cell **> bufferCell; > > + EdgeBuffer<SlotRef> bufferSlot; > > + EdgeBuffer<Value *> bufferVal; > > + EdgeBuffer<Cell **> bufferRelocCell; > > + EdgeBuffer<Value *> bufferRelocVal; > > Need a comment somewhere that the Reloc ones are the ones where elements can > be removed before GC happens. Added a comment above the new RelocatableMonoTypeBuffer. > @@ +300,5 @@ > > + if (!enabled) > > + return; > > + bufferCell.put(o); > > + } > > + void putSlot(JSObject *obj, uint32_t slotno) { > > |slotno| -> |slot|, please. I could have sworn I fixed this already in HeapSlot, but it was still slotno. Fixed now. > ::: js/src/jsgc.cpp > @@ +3100,5 @@ > > MarkRuntime(gcmarker); > > } > > > > +static bool > > +MarkWeakIteratively(JSTracer *trc) > > What's the reason for factoring this out? MarkWeakReferences calls drainMarkStack internally, so cannot be directly used by GCNursery. I split out the non-GCMarker requiring bits so I could re-use it in GCNursery. I was thinking for some reason here that I would need to MarkRuntime and MarkWeakIteratively for the verifier. This is wrong and I have removed it now. > @@ +4666,5 @@ > > + * 2) Step until it becomes == <value of tgt> > > + * 3) Figure out why this assignment is not inserting loc into into the > > + * store buffer. > > + */ > > + JS_ASSERT(comp->gcStoreBuffer.containsEdgeAt(loc)); > > Please see how the pre-barrier verifier asserts now. It prints friendlier > messages so that the fuzzers can sort out different versions of the > assertion. Oooo, very nice. :-) > @@ +4692,5 @@ > > + trc->count = 0; > > + > > + PurgeRuntime(trc); > > + > > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > > Please use |c|, rather than |comps|, since that's what we always do. I noticed that I actually used the correct formulation later in the function. Not sure what happened there. > @@ +4695,5 @@ > > + > > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > > + JSCompartment *comp = comps.get(); > > + comp->gcStoreBuffer.coalesceForVerification(); > > + comp->setNeedsBarrier(false); > > What's the setNeedsBarrier for? I think I may have just needed a clobber, since it's not happening anymore. Previously, it was falling over in a pre barrier when c->needsBarrier() was true and some other invariant was asserting false. > @@ +4698,5 @@ > > + comp->gcStoreBuffer.coalesceForVerification(); > > + comp->setNeedsBarrier(false); > > + } > > + > > + /* Walk the heap. Note: we MarkRuntime in GCNursery, so don't visit it. */ I expanded on this comment. I was attempting to note that the reason we do not mark through the items stored in the runtime during verification is that we mark these items explicitly during GCNursery. > @@ +4703,5 @@ > > + for (CompartmentsIter comps(rt); !comps.done(); comps.next()) { > > + JSCompartment *comp = comps.get(); > > + if (comp->watchpointMap) > > + comp->watchpointMap->markAll(trc); > > + comp->markCrossCompartmentWrappers(trc); > > Why do you need to markCrossCompartmentWrappers? It expects only to be > called during a real GC. I cargo culted both of these into the previous verifier and apparently didn't notice them as I read this immediately after writing the above comment. *facepalm* In any case, I think it's in much better shape now and ready for a second review pass.

With these barriers added, about 2/3 of the jit-tests pass in the interpreter. I will open a new bug for this later, just posting it here so it is easy to find while I am out of the office next week.

Comment on attachment 637298 [details] [diff] [review] v2: With review commentary applied. Review of attachment 637298 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +246,5 @@ > return true; > } > > static JSBool > VerifyBarriers(JSContext *cx, unsigned argc, jsval *vp) We barely use verifybarriers in the test suite. I think it would be better to make verifyprebarriers() and verifypostbarriers() and to convert verifybarriers -> verifyprebarriers in the tests. ::: js/src/gc/StoreBuffer.cpp @@ +271,5 @@ > + if (!edgeSet.initialized()) { > + if (!edgeSet.init()) > + return false; > + } > + JS_ASSERT(edgeSet.count() == 0); I think you want empty(). @@ +294,5 @@ > + > +void > +StoreBuffer::releaseVerificationData() > +{ > + edgeSet.clear(); You probably want to use finish() instead of clear(), since finish() releases memory. ::: js/src/gc/StoreBuffer.h @@ +36,5 @@ > + > + void disable() { > + if (!nursery.initialized()) > + return; > + nursery.clear(); You might want to use finish() instead of clear(), since finish() releases memory. @@ +44,5 @@ > + > + bool isInside(void *cell) const { return nursery.has(cell); } > + > + void insertPointer(void *cell) { > + if (nursery.initialized()) Assuming we cancel verification if enable() fails, this check shouldn't be needed. @@ +201,5 @@ > + void clear(); > + > + /* Check if a pointer is present in the buffer. */ > + bool containsEdge(void *location) const; > + size_t count() const; This method doesn't appear to be used. @@ +291,5 @@ > + bool operator!=(const SlotEdge &other) const { > + return object != other.object || offset != other.offset; > + } > + > + static Value *locationOf(const SlotEdge &sr) { Why is this static? Also, if you return a HeapSlot*, you'll need fewer casts. @@ +293,5 @@ > + } > + > + static Value *locationOf(const SlotEdge &sr) { > + if (sr.object->isDenseArray() && sr.offset < sr.object->getDenseArrayInitializedLength()) > + return (Value *)&sr.object->getDenseArrayElement(sr.offset); I think this is wrong. It should be: if (sr.object->isDenseArray()) { if (sr.offset < sr.object->getDenseArrayInitializedLength()) return (Value *)&sr.object->getDenseArrayElement(sr.offset); return NULL; } @@ +299,5 @@ > + return NULL; > + return (Value *)sr.object->getSlotRef(sr.offset).unsafeGet(); > + } > + > + static void *deref(const SlotEdge &sr) { And why is this static? @@ +301,5 @@ > + } > + > + static void *deref(const SlotEdge &sr) { > + Value *loc = locationOf(sr); > + return loc->isGCThing() ? loc->toGCThing() : NULL; Since locationOf can return NULL, you need to check for that. @@ +349,5 @@ > + bool enable(); > + void disable(); > + > + /* Remove all entries from the store buffer. */ > + void clear(); As far as I can tell, neither this clear method nor any of the clear methods in the buffers are used. ::: js/src/jsgc.cpp @@ +3844,4 @@ > { > + restartPreVerifier = !isShutdown && rt->gcVerifyPreData; > + restartPostVerifier = !isShutdown && rt->gcVerifyPostData; > + if (restartPreVerifier) This should just be rt->gcVerifyPreData, since we always want to end verification before a GC. @@ +3845,5 @@ > + restartPreVerifier = !isShutdown && rt->gcVerifyPreData; > + restartPostVerifier = !isShutdown && rt->gcVerifyPostData; > + if (restartPreVerifier) > + EndVerifyPreBarriers(rt); > + if (restartPostVerifier) Similar change here. @@ +4647,5 @@ > +#ifdef JSGC_GENERATIONAL > + if (rt->gcVerifyPostData || rt->gcIncrementalState != NO_INCREMENTAL) > + return; > + rt->gcVerifyPostData = new (js_malloc(sizeof(VerifyPostTracer))) VerifyPostTracer; > + VerifyPostTracer *trc = (VerifyPostTracer *)rt->gcVerifyPostData; If you put the result of the malloc in |trc| and then copy trc to rt->gcVerifyPostData, then you won't need a cast. @@ +4652,5 @@ > + rt->gcNumber++; > + trc->number = rt->gcNumber; > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > + if (!IsAtomsCompartment(c)) { > + c->gcNursery.enable(); This should check for failure. @@ +4714,5 @@ > + VerifyPostTracer *trc = (VerifyPostTracer *)rt->gcVerifyPostData; > + JS_TracerInit(trc, rt, PostVerifierVisitEdge); > + trc->count = 0; > + > + PurgeRuntime(trc); What's this for? It should have a comment. @@ +4730,5 @@ > + * roots. This is why we can skip MarkRuntime here. > + */ > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > + if (c->watchpointMap) > + c->watchpointMap->markAll(trc); It doesn't really make sense to call this here, since trc->src doesn't have a well-defined value. However, I think you're trying to ensure that locations in the watchpoint map get put in the store buffer, which makes sense. To do that, I think you want to move the |comp->gcNursery.isInside(src)| check out of PostVerifierVisitEdge and put it inside the loop over all cells. That will also allow you to eliminate the |src| field of |trc|, which would be nice. @@ +4732,5 @@ > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > + if (c->watchpointMap) > + c->watchpointMap->markAll(trc); > + if (c->activeAnalysis) > + c->markTypes(trc); markTypes is only supposed to be called from a GC. Also, I don't think it makes sense to call it here. It just marks a bunch of cells, and you're already iterating over all cells down below. @@ +4785,5 @@ > + > +void > +VerifyBarriers(JSRuntime *rt, VerifierType type) > +{ > + if (type == PreBarrierVerifier) { No need for braces here.

This adds almost all of the other needed barriers and also fixes several of the more major problems Bill found in review: enough to get all of the jit-tests passing.

(In reply to Bill McCloskey (:billm) from comment #6) > ::: js/src/builtin/TestingFunctions.cpp > @@ +246,5 @@ > > return true; > > } > > > > static JSBool > > VerifyBarriers(JSContext *cx, unsigned argc, jsval *vp) Agreed. To clarify, would you still like to have a single gc::VerifyBarriers that takes an enum? > @@ +44,5 @@ > > + > > + bool isInside(void *cell) const { return nursery.has(cell); } > > + > > + void insertPointer(void *cell) { > > + if (nursery.initialized()) > > Assuming we cancel verification if enable() fails, this check shouldn't be > needed. This was to get around the fact that we only enable/disable the nursery on compartments that actually exist at the time: I've now added code to test and enable at compartment creation time if we are in verification on the runtime. > @@ +293,5 @@ > > + } > > + > > + static Value *locationOf(const SlotEdge &sr) { > > + if (sr.object->isDenseArray() && sr.offset < sr.object->getDenseArrayInitializedLength()) > > + return (Value *)&sr.object->getDenseArrayElement(sr.offset); > > I think this is wrong. It should be: > > if (sr.object->isDenseArray()) { > if (sr.offset < sr.object->getDenseArrayInitializedLength()) > return (Value *)&sr.object->getDenseArrayElement(sr.offset); > return NULL; > } For dense arrays, slotSpan() is always 0 (it is asserted as much), thus the next check: if (sr.offset >= sr.object->slotSpan()) return NULL; This check is implicitly doing double duty to get the same result as the new one above. I've been meaning to rewrite this method in a way that's not totally insane, of course, so thanks for doing that! > @@ +349,5 @@ > > + bool enable(); > > + void disable(); > > + > > + /* Remove all entries from the store buffer. */ > > + void clear(); > > As far as I can tell, neither this clear method nor any of the clear methods > in the buffers are used. More breakage from where I added enable/disable. We will eventually want this for after a nursery GC: killed off until then. > @@ +4652,5 @@ > > + rt->gcNumber++; > > + trc->number = rt->gcNumber; > > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > > + if (!IsAtomsCompartment(c)) { > > + c->gcNursery.enable(); > > This should check for failure. Good catch. I've moved the nursery/storebuffer disable calls below the 'oom:' label in EndVerifyPostBarriers and copied that trailer up to StartVerifyPostBarriers. > @@ +4714,5 @@ > > + VerifyPostTracer *trc = (VerifyPostTracer *)rt->gcVerifyPostData; > > + JS_TracerInit(trc, rt, PostVerifierVisitEdge); > > + trc->count = 0; > > + > > + PurgeRuntime(trc); > > What's this for? It should have a comment. I cargo-culted it in from the pre barrier verifier. I've been meaning to ask you what it does there. > @@ +4730,5 @@ > > + * roots. This is why we can skip MarkRuntime here. > > + */ > > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > > + if (c->watchpointMap) > > + c->watchpointMap->markAll(trc); > > It doesn't really make sense to call this here, since trc->src doesn't have > a well-defined value. However, I think you're trying to ensure that > locations in the watchpoint map get put in the store buffer, which makes > sense. To do that, I think you want to move the > |comp->gcNursery.isInside(src)| check out of PostVerifierVisitEdge and put > it inside the loop over all cells. That will also allow you to eliminate the > |src| field of |trc|, which would be nice. I like this: I was just using a NULL src. There are actually a couple of things with verification of the watchpoint map that are very strange indeed. I'll try and catch you on IRC later to figure out if what I'm seeing and what I've done are reasonable. Also, the verifier is still missing code to verify the weakmaps and the debugger maps (and incorrectly passing jit-tests because of this). I'm a bit confused by what these need exactly, so let me see if I understand the problem correctly at least. For nursery GCs, we have 4 classes of things we need to mark: stack roots, pointers between heaps, stuff on the runtime, and weakmaps. The barrier verifier mainly cares about interheap pointers and does not care about stack roots or the runtime. The weakmaps are a weird case because some edges into them are present from the runtime: those things we mark because of how the key or value has been marked already and some of the edges into them are there because they are held by something else. Given this, it's not clear to me that we need/want store buffer entries for everything in the nursery that is in the weakmap. What exactly should the verifier be checking for these? > @@ +4732,5 @@ > > + for (CompartmentsIter c(rt); !c.done(); c.next()) { > > + if (c->watchpointMap) > > + c->watchpointMap->markAll(trc); > > + if (c->activeAnalysis) > > + c->markTypes(trc); > > markTypes is only supposed to be called from a GC. Also, I don't think it > makes sense to call it here. It just marks a bunch of cells, and you're > already iterating over all cells down below. This actually asserts, because it uses rt->gcMarker, rather than the trc that it gets passed. In any case, this should just die.

After tracking down why the weakmaps were failing this afternoon all of the related tests started correctly failing.

Comment on attachment 643057 [details] [diff] [review] v4: rebased Review of attachment 643057 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just some nits. ::: js/src/gc/StoreBuffer.cpp @@ +82,5 @@ > + T *cursor = base; > + while (cursor != pos) { > + T edge = *cursor++; > + > + /* Note: the relocatable buffer is allowed to store pointers to 0's. */ No apostrophe after 0. I would just say null. @@ +84,5 @@ > + T edge = *cursor++; > + > + /* Note: the relocatable buffer is allowed to store pointers to 0's. */ > + if (!edge.isNullEdge()) > + continue; This is backwards. @@ +228,5 @@ > + > +void > +StoreBuffer::setNeedsNurseryGC() > +{ > + nursery->setNeedsGC(); None of this setNeedsGC/setNeedsNurseryGC stuff is used, as far as I can tell. Can you remove it? ::: js/src/gc/StoreBuffer.h @@ +18,5 @@ > +namespace gc { > + > +/* > + * Note: this is a stub Nursery that does not actually contain a heap, just a > + * set of pointers which are "inside" the nursery to implement zeal. "... to implement verification." @@ +63,5 @@ > +}; > + > +/* > + * HashKeyRef represents a reference to a HashTable key. Manual HashTable > + * barriers should instance this class with their own table/key type to insert "instance" -> "derive from" @@ +89,5 @@ > +}; > + > +/* > + * The StoreBuffer observes all writes that occur in the system and performs > + * efficient filtering of them to derive a RememberedSet for NurseryGC. Unless RememberedSet and NurseryGC refer to symbols in the program, please just write "remembered set" and "nursery GC". @@ +124,5 @@ > + T *pos; /* Pointer to the current insertion position. */ > + T *top; /* Pointer to one element after the end. */ > + > + MonoTypeBuffer(StoreBuffer *owner_, Nursery *nursery_) > + : owner(owner_), nursery(nursery_), base(NULL), pos(NULL), top(NULL) You don't need these underscores. The initialization syntax works even if the argument names are the same as the field names. @@ +133,5 @@ > + bool enable(uint8_t *region, size_t len); > + void disable(); > + > + bool isEmpty() const { return pos == base; } > + bool isFull() const { return pos >= top; } Can you make this { JS_ASSERT(pos <= top); return pos == top; } @@ +184,5 @@ > + uint8_t *pos; /* Pointer to current buffer position. */ > + uint8_t *top; /* Pointer to one past the last entry. */ > + > + GenericBuffer(StoreBuffer *owner_, Nursery *nursery_) > + : owner(owner_), nursery(nursery_) Same thing here with the underscores. @@ +202,5 @@ > + if (!pos) > + return; > + > + /* Check for overflow. */ > + if (uintptr_t(pos) + sizeof(unsigned) + sizeof(T) >= uintptr_t(top)) { No uintptr_t casts needed. Also, can't this be >, not >=? And it's probably safer to check if (top - pos < sizeof(unsigned) + sizeof(T)). @@ +235,5 @@ > + return !n->isInside(edge) && n->isInside(*edge); > + } > + > + bool isNullEdge() const { > + return *edge; This is backwards. @@ +263,5 @@ > + return !n->isInside(edge) && n->isInside(deref()); > + } > + > + bool isNullEdge() const { > + return deref(); This is backwards. @@ +279,5 @@ > + > + JSObject *object; > + uint32_t offset; > + > + SlotEdge(JSObject *object_, uint32_t offset_) : object(object_), offset(offset_) {} Same underscore thing here. @@ +289,5 @@ > + bool operator!=(const SlotEdge &other) const { > + return object != other.object || offset != other.offset; > + } > + > + HeapSlot *locationOf() const { How about calling this slotLocation? @@ +314,5 @@ > + return !n->isInside(object) && n->isInside(deref()); > + } > + > + bool isNullEdge() const { > + return deref(); Backwards. ::: js/src/jsapi.h @@ +3708,4 @@ > # define JS_SET_TRACING_LOCATION(trc, location) \ > JS_BEGIN_MACRO \ > + if ((trc)->realLocation == NULL || (location) == NULL) \ > + (trc)->realLocation = (location); \ Could you explain this change? Are there times when we set realLocation twice, and it's wrong the second time? ::: js/src/jscompartment.cpp @@ +92,5 @@ > if (!regExps.init(cx)) > return false; > > +#ifdef JSGC_GENERATIONAL > + if (rt->gcVerifyPostData) { Could you comment this code? Is this for the case where a compartment gets created during verification? ::: js/src/jsgc.cpp @@ +4783,5 @@ > +struct VerifyPostTracer : JSTracer { > + /* The gcNumber when the verification began. */ > + uint64_t number; > + > + /* This counts up to JS_VERIFIER_FREQ to decide whether to verify. */ I realize now that this comment and the one for the pre-barrier verifier are wrong. We now use gcZealFrequency instead of JS_VERIFIER_FREQ. Could you fix? @@ +4876,5 @@ > +static void > +EndVerifyPostBarriers(JSRuntime *rt) > +{ > +#ifdef JSGC_GENERATIONAL > + AutoLockGC lock(rt); I think this should not be needed anymore. @@ +4881,5 @@ > + AutoTraceSession session(rt); > + > + rt->gcHelperThread.waitBackgroundSweepOrAllocEnd(); > + > + AutoUnlockGC unlock(rt); Or this. @@ +4912,5 @@ > + c->watchpointMap->markAll(trc); > + > + /* Skip verification if the buffer overflowed. */ > + if (c->gcStoreBuffer.hasOverflowed()) > + continue; You already did this above. @@ +4976,5 @@ > + return; > + > + EndVerifyPreBarriers(rt); > + } > + if (rt->gcZeal() == ZealVerifierPreValue) No need for this test. We already have one above. @@ +4992,5 @@ > + return; > + > + EndVerifyPostBarriers(rt); > + } > + if (rt->gcZeal() == ZealVerifierPostValue) Or this one.

(In reply to Bill McCloskey (:billm) from comment #12) > @@ +228,5 @@ > > + > > +void > > +StoreBuffer::setNeedsNurseryGC() > > +{ > > + nursery->setNeedsGC(); > > None of this setNeedsGC/setNeedsNurseryGC stuff is used, as far as I can > tell. Can you remove it? You are right: NurseryGC scheduling is going to have to come later anyway. > @@ +63,5 @@ > > +}; > > + > > +/* > > + * HashKeyRef represents a reference to a HashTable key. Manual HashTable > > + * barriers should instance this class with their own table/key type to insert > > "instance" -> "derive from" We actually use this class directly (with an appropriate template specialization), so I don't think "derive from" is correct. Maybe this should be "template specialization" to make it clearer? > ::: js/src/jsapi.h > @@ +3708,4 @@ > > # define JS_SET_TRACING_LOCATION(trc, location) \ > > JS_BEGIN_MACRO \ > > + if ((trc)->realLocation == NULL || (location) == NULL) \ > > + (trc)->realLocation = (location); \ > > Could you explain this change? Are there times when we set realLocation > twice, and it's wrong the second time? You are correct. MarkValue calls SET_TRACING_LOCATION internally, since it has to unpack the GCThing pointer to mark. However, in cases where we need the unmoved value to test if we've moved (as for hashtable keys so we know to rekey), we have to copy the Value itself out of its naive location and call SET_TRACING_LOCATION ourself. In this case, we do not want the MarkValueInternal location to override the real location, which we have already set. > ::: js/src/jscompartment.cpp > @@ +92,5 @@ > > if (!regExps.init(cx)) > > return false; > > > > +#ifdef JSGC_GENERATIONAL > > + if (rt->gcVerifyPostData) { > > Could you comment this code? Is this for the case where a compartment gets > created during verification? Correct. > ::: js/src/jsgc.cpp > @@ +4783,5 @@ > > +struct VerifyPostTracer : JSTracer { > > + /* The gcNumber when the verification began. */ > > + uint64_t number; > > + > > + /* This counts up to JS_VERIFIER_FREQ to decide whether to verify. */ > > I realize now that this comment and the one for the pre-barrier verifier are > wrong. We now use gcZealFrequency instead of JS_VERIFIER_FREQ. Could you fix? Oh, I forgot to ask about that. Will fix it. > @@ +4876,5 @@ > > +static void > > +EndVerifyPostBarriers(JSRuntime *rt) > > +{ > > +#ifdef JSGC_GENERATIONAL > > + AutoLockGC lock(rt); > > I think this should not be needed anymore. Cargo culted it in. > @@ +4881,5 @@ > > + AutoTraceSession session(rt); > > + > > + rt->gcHelperThread.waitBackgroundSweepOrAllocEnd(); > > + > > + AutoUnlockGC unlock(rt); > > Or this. Was in the same shipment.

> We actually use this class directly (with an appropriate template specialization), so I don't > think "derive from" is correct. Maybe this should be "template specialization" to make it > clearer? Sorry, I misread. Can you change it to "should instantiate this template"? > You are correct. MarkValue calls SET_TRACING_LOCATION internally, since it has to unpack > the GCThing pointer to mark. However, in cases where we need the unmoved value to test if > we've moved (as for hashtable keys so we know to rekey), we have to copy the Value itself > out of its naive location and call SET_TRACING_LOCATION ourself. In this case, we do not > want the MarkValueInternal location to override the real location, which we have already > set. What's an example?

(In reply to Bill McCloskey (:billm) from comment #14) > > You are correct. MarkValue calls SET_TRACING_LOCATION internally, since it has to unpack > > the GCThing pointer to mark. However, in cases where we need the unmoved value to test if > > we've moved (as for hashtable keys so we know to rekey), we have to copy the Value itself > > out of its naive location and call SET_TRACING_LOCATION ourself. In this case, we do not > > want the MarkValueInternal location to override the real location, which we have already > > set. > > What's an example? HashableValue HashableValue::mark(JSTracer *trc) const { HashableValue hv(*this); JS_SET_TRACING_LOCATION(trc, (void *)this); gc::MarkValue(trc, &hv.value, "key"); return hv; } static inline void MarkValueInternal(JSTracer *trc, Value *v) { if (v->isMarkable()) { JS_ASSERT(v->toGCThing()); void *thing = v->toGCThing(); JS_SET_TRACING_LOCATION(trc, (void *)v); MarkKind(trc, &thing, v->gcKind()); if (v->isString()) v->setString((JSString *)thing); else v->setObjectOrNull((JSObject *)thing); } else { JS_SET_TRACING_LOCATION(trc, NULL); } } In this case, we copy the value to the stack twice. Once to keep from marking it inside the key (we could change this if we added a rekeyFront that doesn't check validity) and a second time when unpacking. Only the first one of these has the real location. The v in MarkValueInternal has the stack location from the first one. Also note the hideous else clause which resets it to null if we did not mark. Normally, realLocation is set to NULL unconditionally after calling the trace function, but if v is not isMarkable, then this won't get called and we will have the wrong location for the next mark call. This, naturally, leads to some extremely confused debugging if we don't get this right. There are only a handful of places where we really need this, more than we currently have. For example, setting realLocation for tracing id's is not necessary because they are never in the nursery. Sadly, I don't think we can do without some sort of similar mechanism because even if we fix all the rekey users, we still have things like the private fields that store objects and, of course, MarkValueInternal. Is this what you were asking, or did I misunderstand the question?

Yeah, that's what I was interested in. It seems like we could just fix that example. But since Jason is going to rewrite it soon anyway, I guess we can put it off. Could you maybe put a FIXME comment at the definition of JS_SET_TRACING_LOCATION to say we want to simplify this in the future?

And backed out in: https://hg.mozilla.org/integration/mozilla-inbound/rev/166cae54928d This insanity: /usr/bin/ccache /tools/android-ndk-r5c/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin/arm-linux-androideabi-g++ -o WyciwygChannelParent.o -c ... /netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp cc1plus: warnings being treated as errors In file included from ../../../dist/include/xpcpublic.h:16, from ../../../dist/include/dombindings.h:14, from /builds/slave/m-in-andrd-dbg/build/content/events/src/nsDOMTouchEvent.h:14, from ../../../dist/include/IPC/nsGUIEventIPC.h:11, from ../../../ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowser.h:22, from ../../../ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowserChild.h:10, from ../../../dist/include/mozilla/dom/TabChild.h:12, from /builds/slave/m-in-andrd-dbg/build/netwerk/protocol/websocket/WebSocketChannelChild.cpp:9: ../../../dist/include/jsgc.h: In function 'bool js::gc::IsNurseryAllocable(js::gc::AllocKind)': ../../../dist/include/jsgc.h:118: error: comparison between signed and unsigned integer expressions make[7]: *** [WebSocketChannelChild.o] Error 1

Relanded in: https://hg.mozilla.org/integration/mozilla-inbound/rev/139a8f2a8538