swb: annotate BaseDictionary (partial) #1797

jianchun · 2016-10-21T01:58:48Z

Expand Pointer macro to choose WriteBarrierPtr/NoWriteBarrierPtr
based on optional Allocator type using WriteBarrierPtrTraits.
(E.g. pointer fields but using with Arena, choose NoWriteBarrierPtr.)

Add WriteBarrier function to trigger write barrier on array content
when needed.

Added CopyArray function to copy array data. Triggers write barrier
on the array content when needed.

By default only pointer types, WriteBarrierPtr wrapper, or explicit
_write_barrier_policy trigger write barrier (when using Recycler).
Users can also use template instantiation to alter custom allocator/type
write barrier policy.

This change does not handle individual Item write barrier yet.

jianchun · 2016-10-21T16:35:08Z

Please hold on review -- Curtis suggested a better approach. Will update.

jianchun · 2016-10-21T21:18:33Z

@dotnet-bot test Linux tests please

jianchun · 2016-10-21T22:35:14Z

@leirocks @curtisman @digitalinfinity Please CR, thanks!

digitalinfinity · 2016-10-22T00:16:55Z

lib/Common/DataStructures/BaseDictionary.h

+        Pointer(int, TAllocator) buckets;
+        Pointer(EntryType, TAllocator) entries;
+        PointerNoBarrier(AllocatorType) alloc;
        int size;


For consistency, it's better if we annotate all the fields (so annotate the non-pointer fields with Field)

Yes, will do (paid too much attention to buckets and entries).

digitalinfinity · 2016-10-22T00:25:16Z

lib/Common/DataStructures/BaseDictionary.h

+            Allocate(&newBuckets, &newEntries, initBucketCount, initSize);
+
+            // Allocation can throw - assign only after allocation has succeeded.
+            SetArray<TAllocator>(this->buckets, newBuckets, initBucketCount);


This is a little counterintuitive- you're setting a field in the dictionary, not the array itself, so you don't need to trigger the write barrier for every element in the array. You could argue that SetArray is getting called here because Allocate was writing to every element in the array but Allocate just does a memset without any pointers so it's probably fine to not trigger the barrier here. So in short- any reason why you can't just do this->buckets = newBuckets here?

The write barrier on elements scenario is hard to understand (I'm still not too clear after asking Lei and Curtis.) this->buckets = newBuckets is fine here, but this->entries = newEntries is not. I was thinking of how we prevent accidental error. In the case of pointer field, the operator= function takes care of all scenarios, be it raw pointer, NoWriteBarrierPtr, or WriteBarrierPtr. So in comparison, I was thinking maybe we require all array assignment through this SetArray function. SetArray determines if we need write barrier or not. Possibly we can track that in clang plugin (not trivial...).

Why is this->entries = newEntries not fine? this->entries and this->buckets are both pointer types- that is, setting them goes through the barrier on the Dictionary instance itself, not on the entries. If you assign this->entries = blah, you don't need to see all the bits corresponding to the range of memory symbolized by the entries array, you need to only set the bit corresponding to the dictionary itself, since that's the memory that has changed. That being said, you'll need to do what SetArray does when you do something like memcpy or set individual elements in the array.

Yes, this->entries = newEntries is fine when newEntries already takes care of write barrier on the array elements. So either I do it here with SetArray, or I do something similar ahead replacing js_memcpy_s with a version of CopyArray. Either approach has the concern that it is hard to know we missed one. Do you think CopyArray is safer in terms of write barrier correctness? e.g. is it possible GC collects in between js_memcpy_s and my current SetArray, and SetArray write barrier is too late?

Making this change now. Revert SetArray, prefer earlier with CopyArray.

digitalinfinity · 2016-10-22T00:26:10Z

        js_memcpy_s(newEntries, sizeof(EntryType) * newSize, entries, sizeof(EntryType) * count);

This probably needs the write barrier but I think you already noted that you haven't set barriers on the items themselves

Refers to: lib/Common/DataStructures/BaseDictionary.h:1020 in 9958e2f. [](commit_id = 9958e2f, deletion_comment = False)

digitalinfinity · 2016-10-22T00:26:48Z

lib/Common/Memory/RecyclerWriteBarrierManager.cpp


 void
-RecyclerWriteBarrierManager::WriteBarrier(void * address, size_t ptrCount)
+RecyclerWriteBarrierManager::WriteBarrier(void * address, size_t bytes)


Curious- why this change?

Dictionary's "entries" is an array of "EntryType" structs, not pointers.

digitalinfinity · 2016-10-22T00:27:44Z

lib/Common/Memory/RecyclerPointers.h

+class Recycler;
+class RecyclerNonLeafAllocator;
+
+// Dummy tag classes to mark yes/no write barrier policy


Clever- although has the potential to be a little confusing so it's worth clearly documenting this

Agree, looks quite complex to myself...

digitalinfinity · 2016-10-22T00:27:47Z

jianchun · 2016-10-22T00:50:47Z

        js_memcpy_s(newEntries, sizeof(EntryType) * newSize, entries, sizeof(EntryType) * count);

I was hoping line 1046 below will take care of that. I was unsure if it would be a problem around line 1039 where we DeleteEntries(entries, ...). But line 1046 will write barrier the whole array content. Would it work?

In reply to: 255494454 [](ancestors = 255494454)

Refers to: lib/Common/DataStructures/BaseDictionary.h:1020 in 9958e2f. [](commit_id = 9958e2f, deletion_comment = False)

digitalinfinity · 2016-10-25T00:00:03Z

LGTM

Expand `Pointer` macro to choose `WriteBarrierPtr/NoWriteBarrierPtr` based on optional `Allocator` type using `WriteBarrierPtrTraits`. (E.g. pointer fields but using with Arena, choose NoWriteBarrierPtr.) Add `WriteBarrier` function to trigger write barrier on array content when needed. Added `CopyArray` function to copy array data. Triggers write barrier on the array content when needed. By default only pointer types, WriteBarrierPtr wrapper, or explicit _write_barrier_policy trigger write barrier (when using Recycler). Users can also use template instantiation to alter custom allocator/type write barrier policy. This change does not handle individual `Item` write barrier yet.

jianchun · 2016-10-25T01:08:23Z

Thanks @digitalinfinity !

Merge pull request #1797 from jianchun:wballoc Expand `Pointer` macro to choose `WriteBarrierPtr/NoWriteBarrierPtr` based on optional `Allocator` type using `WriteBarrierPtrTraits`. (E.g. pointer fields but using with Arena, choose NoWriteBarrierPtr.) Add `WriteBarrier` function to trigger write barrier on array content when needed. Added `CopyArray` function to copy array data. Triggers write barrier on the array content when needed. By default only pointer types, WriteBarrierPtr wrapper, or explicit _write_barrier_policy trigger write barrier (when using Recycler). Users can also use template instantiation to alter custom allocator/type write barrier policy. This change does not handle individual `Item` write barrier yet.

msftclas added the cla-not-required label Oct 21, 2016

jianchun force-pushed the wballoc branch from 1237c8d to 9958e2f Compare October 21, 2016 21:18

digitalinfinity reviewed Oct 22, 2016

View reviewed changes

jianchun force-pushed the wballoc branch from d5fc8fc to d2a2eef Compare October 24, 2016 21:45

jianchun force-pushed the wballoc branch from d2a2eef to e26f96b Compare October 25, 2016 00:38

jianchun merged commit e26f96b into chakra-core:swb Oct 25, 2016

jianchun deleted the wballoc branch October 25, 2016 01:11

swb: annotate BaseDictionary (partial) #1797

swb: annotate BaseDictionary (partial) #1797

Uh oh!

Conversation

jianchun commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jianchun commented Oct 21, 2016

Uh oh!

jianchun commented Oct 21, 2016

Uh oh!

jianchun commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalinfinity commented Oct 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digitalinfinity commented Oct 22, 2016

Uh oh!

jianchun commented Oct 22, 2016

Uh oh!

digitalinfinity commented Oct 25, 2016

Uh oh!

jianchun commented Oct 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jianchun commented Oct 21, 2016 •

edited

Loading