Add support for a Recycler-managed "Host Heap" to facilitate tracing of objects which relate to scriptable types #3846

BoCupp · 2017-10-02T03:35:17Z

This change exposes functions that will allocate memory inside of Chakra's GC. This memory can be precisely traced or leaf, and either finalizable or not. Traced objects will have IRecyclerVisitedObject::Trace called on them during ProcessMark if they were added to a new type of mark stack during Mark. Trace implementations will use a new API to tell the marking context to add a set of pointers to the appropriate mark stack.

Finalizable objects will have Dispose called when the the object is no longer reachable, in order to perform unmanaged resource cleanup.

These objects (except for non-finalizable leaf objects) will all live inside a new heap block type. Currently there is only support for small and medium blocks.

GCStress has been updated to allocate objects of the various new types. Finalizable objects hold onto some unmanaged heap resources which would point out (via OOM) if objects are not correctly being cleaned up.

More details:

Add support for small and medium RecyclerVisitedHostHeapBlockTypes
- For now, add failfast for RecyclerVisitedObjects that are placed in the LargeHeapBlock
Publish IRecyclerVisitedObject interface which adds new method (compared to FinalizableObject) for precise tracing support
- The old FinalizableObject is now a specialization of RecyclerVisitedObject and is used internally by Chakra
- For host allocations, the host is expected to provide an appropriate implementation of IRecyclerVisitedObject
Publish IRecyclerHeapMarkingContext (used as the argument to IRecyclerVisitedObject::Trace)
- The host must implement precisely tracing the (non-leaf) memory it allocates by calling IRecyclerHeapMarkingContext::MarkObjects from inside its implementation of IRecyclerVisitedObject::Trace
- It is implemented by a stack wrapper on MarkContext that captures the parallel and interior template parameters from MarkContext::ProcessMark. We always use false for doSpecialMark, since all calls to Mark that originate from ProcessMark would pass false (the only place we pass true is when scanning the stack)
Create Chakra exports for allocation and rootaddref/release functions
Introduce a new RecyclerHeap lib in chakra that defines the exports in the cpp and declares them in the h
Add support for RecyclerVisitedObjects in GCStress

Note: this change is derived from a rebase of work done by Daniel Libby - all credit to him for the work to enable this concept.

Penguinwizzard · 2017-10-04T00:31:15Z

Could you rebase this PR on top of master? We have fixes for the CI issues (some of which you addressed in your own changes) in the current version of master.

digitalinfinity · 2017-10-04T06:25:34Z

lib/Common/Memory/HeapBlock.h

-        MediumAllocBlockTypeCount = 5, // Actual number of types for blocks containing medium allocations
-        SmallBlockTypeCount = 11,      // Distinct block types independent of allocation size using SmallHeapBlockT
+#ifdef RECYCLER_VISITED_HOST
+        SmallAllocBlockTypeCount = 7, // Actual number of types for blocks containing small allocations


There's no test for this today IIRC but you probably need to update JD to handle this update?

Yes, I believe it has been updated, but that code lives outside of ChakraCore and isn't part of this PR, or is there a spot under core that needs to be updated?

digitalinfinity · 2017-10-04T06:25:39Z

lib/Common/Memory/HeapBlockMap.inl

@@ -142,13 +142,38 @@ HeapBlockMap32::Mark(void * candidate, MarkContext * markContext)
 #endif
        ((SmallFinalizableHeapBlock*)chunk->map[id2])->ProcessMarkedObject<doSpecialMark>(candidate, markContext);
        break;
+#ifdef RECYCLER_VISITED_HOST


For visited objects, is mark always called with interior marking mode- if so, do we actually need to implement this or should we just assert/fail fast here?

We do need to implement it. This is the core path. The interior template parameter of MarkingContext::Mark controls if we are using HeapBlockMap*::MarkInterior or HeapBlockMap*::Mark. The interior template parameter though I believe is only true for the MemGC recycler, so the JS GC is always using HeapBlockMap*::Mark.

Inside HeapBlockMap*::Mark we must still support having interior pointers to the new types of blocks we added, since we expect the majority of our pointers to be interior due to our wrapper approach for implementing IRecyclerVisitedObject in the host. As a result, once we know the block type in the HeapBlockMap*::Mark method, we can support interior references by calling MarkInteriorInternal on a computed pointer obtained from GetRealAddressFromInterior.

I think I could assert/fail in the markinterior version. I'll try adding that.

The other place which sets enableScanInteriorPointers (which leads to calling HeapBlockMap*::MarkInterior) is in the Recycler constructor based on a CUSTOM_CONFIG_FLAG. If I force failure under the MarkInterior path then allocating our new type of heap block would lead to a failure in that configuration. Is that OK or should MarkInterior stay as is?

digitalinfinity · 2017-10-04T06:25:49Z

lib/Common/Memory/HeapBucket.cpp

@@ -1617,6 +1623,9 @@ HeapBucketGroup<TBlockAttributes>::ScanInitialImplicitRoots(Recycler * recycler)
    smallFinalizableWithBarrierHeapBucket.ScanInitialImplicitRoots(recycler);
 #endif
    finalizableHeapBucket.ScanInitialImplicitRoots(recycler);
+#ifdef RECYCLER_VISITED_HOST
+    recyclerVisitedHostHeapBucket.ScanInitialImplicitRoots(recycler);


Is this needed? Can visited host objects ever be implicit roots?

I believe only the MemGC allocators set that bit, so this bucket should never contain any "implicit roots". I'll try removing it.

digitalinfinity · 2017-10-04T06:25:53Z

lib/Common/Memory/HeapBucket.cpp

@@ -1651,6 +1663,9 @@ void
 HeapBucketGroup<TBlockAttributes>::SweepFinalizableObjects(RecyclerSweep& recyclerSweep)
 {
    finalizableHeapBucket.Sweep(recyclerSweep);
+#ifdef RECYCLER_VISITED_HOST
+    recyclerVisitedHostHeapBucket.Sweep(recyclerSweep);


Curious- is there a reason that these need to be swept before the smallFinalizableWithBarrier heap bucket? Or was this just an arbitrary choice?

I believe its arbitrary. I think it was because Daniel first added the code without the RECYCLER_VISITED_HOST directives and it looked neater organizing the non-conditionally-compiled code together.

digitalinfinity · 2017-10-04T06:25:57Z

lib/Common/Memory/MarkContext.h

@@ -102,6 +106,7 @@ class MarkContext
    Recycler * recycler;
    PagePool * pagePool;
    PageStack<MarkCandidate> markStack;
+    PageStack<IRecyclerVisitedObject*> preciseStack;


Consider wrapping this in #ifdef RECYCLER_VISITED_HOST?

digitalinfinity · 2017-10-04T06:26:02Z

lib/Common/Memory/Recycler.inl

+Recycler::AddPreciselyTracedMark(IRecyclerVisitedObject * candidate) throw()
+{
+    // This is never called during parallel marking
+    Assert(this->collectionState != CollectionStateParallelMark);


Is this ever called during mark or only during rescan? Can we turn this assert into the positive case instead of the negative case?

Only during rescan. I can change the assert.

Actually, when changing the assert and running GCStress I find that while called under BackgroundRescan the Recycler's collectionState is Collection_ConcurrentMark... which doesn't have the Collection_Rescan flag. I think the important aspect of the assert, however, is captured in that a Recycler API which pushes something into a MarkingContext lacks the information necessary to know which MarkingContext in the case of parallel marking.

I made a minor change to the assert to make this clearer:

// This API cannot be used for parallel marking as we don't have enough information to determine which MarkingContext to use. Assert((this->collectionState & Collection_Parallel) == 0);

digitalinfinity · 2017-10-04T06:26:06Z

lib/Common/Memory/Recycler.inl

 };
 }

 template <ObjectInfoBits attributes, bool nothrow>
 inline char *
 Recycler::AllocWithAttributesInlined(DECLSPEC_GUARD_OVERFLOW size_t size)
 {
-    // All tracked objects are client tracked objects
+    // All tracked objects are client tracked or recycler host visited objects


Confused by this comment- why are host visited objects allocated with the track bit?

We are currently using the TrackBit to indicate an object that needs tracing. All our RecyclerVisitedHostTraced* objects currently have the TrackBit set. Seems like we could have chosen to use "not Leaf" for the same purpose. I'll explore that option.

digitalinfinity · 2017-10-04T06:26:13Z

lib/Common/Memory/SmallFinalizableHeapBlock.cpp

+{
+    bool noOOMDuringMark = true;
+
+    if (attributes & TrackBit)


I don't see any allocation variant in Recycler.h where we can allocated visited host objects with a track bit so when would this condition be true? Wondering if the track/newtrack code can be just removed from this method and treated as assert/failfast?

See comment above on traced objects currently using the TrackBit.

digitalinfinity · 2017-10-04T06:26:20Z

lib/Common/Memory/SmallFinalizableHeapBlock.h

+    typedef TBlockAttributes HeapBlockAttributes;
+
+    static const ObjectInfoBits RequiredAttributes = RecyclerVisitedHostBit;
+    static const bool IsLeafOnly = true;


Why is this true? Also, is this used anywhere...?

digitalinfinity · 2017-10-04T06:26:27Z

lib/Common/Memory/SmallFinalizableHeapBlock.cpp

+        Assert((attributes & LeafBit) == 0);
+        Assert(block->GetAddressIndex(objectAddress) != SmallHeapBlockT<TBlockAttributes>::InvalidAddressBit);
+
+        if (!recycler->AddPreciselyTracedMark(reinterpret_cast<IRecyclerVisitedObject*>(objectAddress)))


Why is this not always called when we rescan a visited object?

Objects are only pushed onto the preciseStack of the MarkContext if they need traced. Since the TrackBit currently indicates whether or not we need traced it should guard AddPreciselyTracedMark.

BoCupp · 2017-10-05T03:53:54Z

Note that I've addressed comments above not having to do with the use of the TrackBit for indicating which RecyclerVisitedObjects need the Trace method called. I'm attempting to switch that to RecyclerVisitedObjects that are not leaves are traced, but GCStress is crashing. When I resolve the issue I'll push again.

Update: My plan was to eliminate using the trackbit to indicate the objects that need precise trace in favor of (attributes & Leaf) == 0, but the ObjectInfo attributes are 0 for free blocks as well and a false positive into a valid heap block's free object slot will actually mark the free slot and push the pointer to the free block into the preciseStack where we will call Trace and crash.

So it looks like we must use a non-zero bit to indicate when we want to trace. I also thought about using the free bits of the block to test whether we are marking a valid object instead of just the ObjectInfo bits, but it doesn't look like the free bits on a newly allocated block can be computed as everything should be free, but the freed object list is null.

digitalinfinity · 2017-10-12T01:04:46Z

lib/Common/Memory/SmallFinalizableHeapBlock.cpp

+template <bool doSpecialMark, typename Fn>
+bool SmallRecyclerVisitedHostHeapBlockT<TBlockAttributes>::UpdateAttributesOfMarkedObjects(MarkContext * markContext, void * objectAddress, size_t objectSize, unsigned char attributes, Fn fn)
+{
+    bool noOOMDuringMark = true;


Ok, so if I understand this correctly, HeapBlock::UpdateAttributesOfMarkedObjects should never be called for HostVisited Heap Blocks, correct? If so, in HeapBlock.inl, in that method, can you add an assert checking that?

Yes that's right. I'll add it.

I pushed the change.

digitalinfinity · 2017-10-12T01:07:35Z

lib/Common/Memory/Recycler.inl

@@ -36,18 +36,22 @@ namespace Memory
 class DummyVTableObject : public FinalizableObject


I think this should probably match the RecyclerVisitedObject vtable layout?

FinalizableObject derives from IRecyclerVisitedObject so it has a matching v-table layout.

Add support for a RecyclerVisitedHostHeapBlock This adds a new heap block type, and a new heap block implementation. Currently this simply derives from SmallFinalizableHeapBlockT and only has minor specialization: SetAttributes has slightly different behavior since we can have non-finalizable objects (traced only) HeapBlockType is set appropriately by a New/ctor implementation Recycler macros are introduced that will allocate a chunk of memory with the appropriate bits but will not construct an object - this memory will eventually be given to the caller to put a v-table on it in order to get the visitation callbacks. Note that true leaf objects (of which we expect to have a few in edgehtml) are not supported in this heap block type - the idea is that they will go into the existing leaf heap block

Add RecyclerVisitedObject interface and publish it FinalizableObject derives from this new interface. New Trace method that takes the markingcontext handle, will be implemented by host recycler visited objects and called from new heap block type FinalizableObject implements a Trace that asserts and is not overridable by chakra types (for now) The new interface is now published to the same location as edgejavascripttypes.h so that edgehtml can include it to implement the vtable

Address code review feedback for new heap block type Remove underscores in recycler macros Remove underscores in heap block bits Add support for leaf into new heap block Remove non-zeroed allocators Update test to allocate leafs in our heapblock

…itedHeapBlockT Specialize ProcessMarkedObject for SmallRecyclerVisitedHeapBlockT For our new heap block type, implement ProcessMarkedObject and UpdateAttributes to call Trace instead of Mark Have interior behavior even for non-interior marking for our heap block type Add missing case to interior mark Add a test that actually traces, and with interior pointers.

Introduce a new RecyclerHeap lib in chakra that defines the exports in the cpp and declares them in the h Include the h in edgehtml where it will be used and delete the old local version of RecyclerHeap.h I validated that linking works (but right now it doesn't link in due to dead symbols removal at link time). Also re-arranged the allocation macros to better fit what we need to do - we'll never be running a constructor, but instead are just returning allocated memory. Cut out the middleman and just call AllocZeroWithAttributesInlined more or less directly

…of IRecyclerVisitedObject::Trace from UpdateAttributes to ProcessMark This allows us to not have recursive marking for our precisely traced objects and makes it easier to reason about what operation the callback that Trace uses (IRecyclerHeapMarkingContext::MarkObjects) performs (it just marks every pointer that it is given. IRecyclerHeapMarkingContext is now published, and is the new argument to IRecyclerVisitedObject::Trace. It is implemented by a stack wrapper on MarkContext that captures the parallel and interior template parameters from MarkContext::ProcessMark. We always use false for doSpecialMark, since all calls to Mark that originate from ProcessMark would pass false (the only place we pass true is when scanning the stack) IRecyclerHeapMarkingContext::MarkObjects currently does an interior mark on the assumption that the pointers provided by the externally precisely traced object are going to be majority interior We also changed to LeafBit designation to be more accurate, logically. Precisely traced objects are not actually leaf since we're tracing them, and since they go into our new heap block, they will not be conservatively scanned regardless of the leaf bit. Removing this causes us to have a couple more heap bucket template instantiations, since we now have 4 unique bit patterns to account for when masking with GetBlockTypeBitMask Update the test to trace through a non-recyclervisited stack root, that also uses an interior pointer (but aligned) to make sure the redirection to our heap block map for (aligned) interior pointers during non-interior marking is working as expected.

… chakra heap for tracing and finalizing Create macros that allow for CBase and other types of objects to use the templated ThreadAlloc's for allocation. Use the macro for WebGLActiveInfo. Convert AddRef/Release to RootAddRef/RootRelease calls Virtual all non-public ref counting methods on CBase and abandon if they are called neuter JSBind_* methods related to refcounting Implement a Trace that simply traces the Var (which we actually know will already be marked due to how this object is reachable) Add a basic leak detector to the new type (interim, more as a sanity check) Modify callers creating this type of object to just new into a raw pointer on the stack. Call Passivate (if exists) in Dispose Ensure pointer is 16-byte aligned so that it is marked from CEO Var Patch up the IRecyclerVisitedObject interface to not take Recycler, but instead encapsulate that inside FinalizableObject Remove assert that prevented interior marking when in parallel/concurrent and recycler state not set (recycler state currently only used by MemGC) Modify assert in CBase::Passivate that expects Var to be nullptr by opting out GC traced objects

…l Trace will never fail with OOM, since that is encapsulated by MarkContext::Mark and we don't expect edgehtml objects to allocate during Trace (or at least not in a way that is recoverable). On top of that there's not really a semantic that makes sense for a failure return in ProcessMark

…eapBlock We're not planning on supporting this in the short-term, failfast if this ever happens

Add support for RecyclerVisitedObjects in GCStress Add a new object type that is weighted similarly to TrackedObject. The new object type will go into the new heap block type, and can be a combination of leaf, finalizable, and traced (the type of the created object is random). We also allocate an unmanaged resource for finalizable objects to ensure that finalize is called and we release the memory (we'll eventually OOM otherwise) Adding this showed that our new heap block type needed its own rescan logic - traced objects must go into the precisely traced mark stack instead of being handled generically by being added to the conservatively scanned mark stack.

…ncepts are mutually exclusive

…VISITED_HOST

…is not defined

…ited objects

… in Ubuntu builds

… for RecyclerVisitedHost HeapBlocks

…ject implementations

dilijev · 2017-10-13T17:39:26Z

@dotnet-bot test Ubuntu static_ubuntu_linux_test please

dilijev · 2017-10-13T17:40:23Z

@mmitche Sometimes we get stuck checks that completed but for some reason failed to report their completed status back to GitHub. Is there a way to force re-sending that status update instead of running the check from scratch?

mmitche · 2017-10-13T17:41:45Z

@dilijev Nope :/

dilijev · 2017-10-13T17:42:08Z

@mmitche Oh well, it's a very minor problem. *shrug*

…to facilitate tracing of objects which relate to scriptable types Merge pull request #3846 from BoCupp:build/pcupp/recycler_visited_host_heap This change exposes functions that will allocate memory inside of Chakra's GC. This memory can be precisely traced or leaf, and either finalizable or not. Traced objects will have IRecyclerVisitedObject::Trace called on them during ProcessMark if they were added to a new type of mark stack during Mark. Trace implementations will use a new API to tell the marking context to add a set of pointers to the appropriate mark stack. Finalizable objects will have Dispose called when the the object is no longer reachable, in order to perform unmanaged resource cleanup. These objects (except for non-finalizable leaf objects) will all live inside a new heap block type. Currently there is only support for small and medium blocks. GCStress has been updated to allocate objects of the various new types. Finalizable objects hold onto some unmanaged heap resources which would point out (via OOM) if objects are not correctly being cleaned up. More details: - Add support for small and medium RecyclerVisitedHostHeapBlockTypes - For now, add failfast for RecyclerVisitedObjects that are placed in the LargeHeapBlock - Publish IRecyclerVisitedObject interface which adds new method (compared to FinalizableObject) for precise tracing support - The old FinalizableObject is now a specialization of RecyclerVisitedObject and is used internally by Chakra - For host allocations, the host is expected to provide an appropriate implementation of IRecyclerVisitedObject - Publish IRecyclerHeapMarkingContext (used as the argument to IRecyclerVisitedObject::Trace) - The host must implement precisely tracing the (non-leaf) memory it allocates by calling IRecyclerHeapMarkingContext::MarkObjects from inside its implementation of IRecyclerVisitedObject::Trace - It is implemented by a stack wrapper on MarkContext that captures the parallel and interior template parameters from MarkContext::ProcessMark. We always use false for doSpecialMark, since all calls to Mark that originate from ProcessMark would pass false (the only place we pass true is when scanning the stack) - Create Chakra exports for allocation and rootaddref/release functions - Introduce a new RecyclerHeap lib in chakra that defines the exports in the cpp and declares them in the h - Add support for RecyclerVisitedObjects in GCStress Note: this change is derived from a rebase of work done by Daniel Libby - all credit to him for the work to enable this concept.

dilijev added the External Contributor label Oct 4, 2017

digitalinfinity reviewed Oct 4, 2017

View reviewed changes

BoCupp force-pushed the build/pcupp/recycler_visited_host_heap branch from 029a036 to a8587fe Compare October 5, 2017 03:52

BoCupp force-pushed the build/pcupp/recycler_visited_host_heap branch 2 times, most recently from c411ae9 to 8cca400 Compare October 10, 2017 01:26

digitalinfinity reviewed Oct 12, 2017

View reviewed changes

dlibby- and others added 12 commits October 12, 2017 14:46

Fix build error due to bad search/replace during patch application

d4e7e08

Forward declare Recycler in RecyclerVisitedObject.h

e4ae746

Add failfast for RecyclerVisitedObjects that are placed in the LargeH…

6f80abb

…eapBlock We're not planning on supporting this in the short-term, failfast if this ever happens

dlibby- and others added 18 commits October 12, 2017 15:42

Fix bad merge in ProcessMark for ARM builds

bac5b27

Fix casing for relatively included header to fix Linux builds

4022579

Rewrite test object code to avoid prefast error

da7f090

Enable Recycler Host Heap on Windows builds only

d2cf5dc

Convert tabs to spaces per style requirements

11d70bc

Address prefast errors triggered in regex code

ebbb2e0

Add another AnalysisAssert for the benefit of prefast checking

44ecceb

Suppress incorrect prefast warning for helpers.cpp

77186c3

Update comment to fix typo

f29659e

Remove recyclerVisitedHostHeapBucket from implicit root scanning - co…

3f13bc1

…ncepts are mutually exclusive

Conditionally compile in MarkContext::preciseStack only for RECYCLER_…

3dd0dcc

…VISITED_HOST

Remove unused static const bool IsLeafOnly from blocks

11f6196

Clarify the intent of an assert in Recycler::AddPreciselyTracedMark

c1774aa

Pre-process out more precise tracing code when RECYCLER_VISITED_HOST …

c4faba8

…is not defined

Eliminate more interaction between implicit root bit and recycler vis…

5356fa6

…ited objects

Fix case sensitivity issue for including RecyclerHeapMarkingContext.h…

2d0adbc

… in Ubuntu builds

Asserting the incorrect UpdateAttributesOfMarkedOjbects is not called…

30fcaf2

… for RecyclerVisitedHost HeapBlocks

Fix OSX build with RECYCLER_VISITED_HOST ifdef check

2c6a7da

digitalinfinity removed the External Contributor label Oct 12, 2017

Add a comment on the overloaded use of the TrackBit

6e782df

BoCupp force-pushed the build/pcupp/recycler_visited_host_heap branch from 1eab92a to 6e782df Compare October 12, 2017 23:44

Remove remaining __stdcall calling convention from IRecyclerVisitedOb…

612cd34

…ject implementations

BoCupp force-pushed the build/pcupp/recycler_visited_host_heap branch from 1f03693 to 612cd34 Compare October 13, 2017 01:12

digitalinfinity approved these changes Oct 13, 2017

View reviewed changes

chakrabot merged commit 612cd34 into chakra-core:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a Recycler-managed "Host Heap" to facilitate tracing of objects which relate to scriptable types #3846

Add support for a Recycler-managed "Host Heap" to facilitate tracing of objects which relate to scriptable types #3846

BoCupp commented Oct 2, 2017 •

edited

Loading

Penguinwizzard commented Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

BoCupp Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 5, 2017

BoCupp Oct 5, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 4, 2017

digitalinfinity Oct 4, 2017

digitalinfinity Oct 4, 2017

BoCupp Oct 5, 2017

BoCupp commented Oct 5, 2017 •

edited

Loading

digitalinfinity Oct 12, 2017

BoCupp Oct 12, 2017

BoCupp Oct 12, 2017

digitalinfinity Oct 12, 2017

BoCupp Oct 12, 2017

dilijev commented Oct 13, 2017

dilijev commented Oct 13, 2017

mmitche commented Oct 13, 2017

dilijev commented Oct 13, 2017 •

edited

Loading

		@@ -36,18 +36,22 @@ namespace Memory
		class DummyVTableObject : public FinalizableObject

Add support for a Recycler-managed "Host Heap" to facilitate tracing of objects which relate to scriptable types #3846

Add support for a Recycler-managed "Host Heap" to facilitate tracing of objects which relate to scriptable types #3846

Conversation

BoCupp commented Oct 2, 2017 • edited Loading

Penguinwizzard commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoCupp commented Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Oct 13, 2017

dilijev commented Oct 13, 2017

mmitche commented Oct 13, 2017

dilijev commented Oct 13, 2017 • edited Loading

BoCupp commented Oct 2, 2017 •

edited

Loading

BoCupp commented Oct 5, 2017 •

edited

Loading

dilijev commented Oct 13, 2017 •

edited

Loading