some changes for SharedArrayBuffer#2939
Conversation
leirocks
commented
May 8, 2017
leirocks
commented
- Due to spec change, SharedArrayBuffer is not transferrable, remove related code
- Hardening the allocation path for OOM case
- Fix a possible race issue in waiter list
| } | ||
| else | ||
| { | ||
| AssertMsg(false, "We should explicitly have a case statement for each object which has detached state."); |
There was a problem hiding this comment.
nit: you can do AssertOrFailFastMsg(state->GetTypeId() != TypeIds_ArrayBuffer, "blah blah"); This will be lot cleaner.
In reply to: 115386419 [](ancestors = 115386419)
| } | ||
| uint SharedContents::Release() | ||
| { | ||
| return InterlockedDecrement(&refCount); |
There was a problem hiding this comment.
assert of failfast of refCount should be 0 to begin with?
| Recycler* recycler = GetType()->GetLibrary()->GetRecycler(); | ||
| recycler->ReportExternalMemoryFree(sharedContents->bufferLength); | ||
| } | ||
|
|
There was a problem hiding this comment.
why this fails on Linux?
There was a problem hiding this comment.
didn't look into the detail but I guess somehow in Linux the finalGC is not called in linux.
There was a problem hiding this comment.
@jianchun maybe I remember wrong but.. do you think this has anything to do with exit time cleanup we were dealing?
There was a problem hiding this comment.
didn't look into the detail but I guess somehow in Linux the finalGC is not called in linux.
If that's the case, shall we make it non _WIN32 only and add TODO investigate?
| long ret = InterlockedDecrement(&refCount); | ||
| if (ret < 0) | ||
| { | ||
| Js::Throw::FatalInternalError(); |
There was a problem hiding this comment.
nit: you can do AssertOrFailFastMsg (ret >= 0).
|
|
| template <typename TAllocator> | ||
| struct ForceNonLeafAllocator | ||
| { | ||
| static const bool FakeZeroLengthArray = true; |
There was a problem hiding this comment.
see 5224e05
while Heap allocation zero length array, we allocated the track data, but we didn't delete the track data in DeleteArray, causes memory leak in chk build.
with changing DeleteArray to use the FakeZeroLengthArray, every allocator need to provide such definition.
| class RecyclerLeafAllocator | ||
| { | ||
| public: | ||
| static const bool FakeZeroLengthArray = true; |
| void DeleteArray(typename AllocatorInfo<TAllocator, T>::AllocatorType * allocator, size_t count, T * obj) | ||
| { | ||
| if (count == 0) | ||
| if (count == 0 && TAllocator::FakeZeroLengthArray) |
There was a problem hiding this comment.
Should this be AllocatorInfo<TAllocator, T>::AllocatorType::FakeZeroLengthArray instead?
But why do we care?
There was a problem hiding this comment.
hmm, could be, then I can remove the FakeZeroLengthArray on RecyclerLeafAllocator I guess.
There was a problem hiding this comment.
AllocatorInfo<TAllocator, T>::AllocatorType is just same as TAllocator...
we care because for Heap allocated zero length array, we still allocate memory for the tracking data. and for Heap allocator the tracking data is in front of the user address, need to call into the free function to clear the tracking data and reset the memory leaking status
leirocks
force-pushed
the
sab1
branch
3 times, most recently
from
7558222 to
a4e46c7
Compare
- Due to spec change, SharedArrayBuffer is not transferrable, remove related code - Hardening the allocation path for OOM case - Fix a possible race issue in waiter list - Enable by default
Merge pull request #2939 from leirocks:sab1 - Due to spec change, SharedArrayBuffer is not transferrable, remove related code - Hardening the allocation path for OOM case - Fix a possible race issue in waiter list - Turn on by default
6 participants
