[webkit-reviews] review canceled: [Bug 196160] [JSC] Butterfly allocation from LargeAllocation should try "realloc" behavior if collector thread is not active : [Attachment 365893] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has canceled Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 196160: [JSC] Butterfly allocation from LargeAllocation should try
"realloc" behavior if collector thread is not active
https://bugs.webkit.org/show_bug.cgi?id=196160

Attachment 365893: Patch

https://bugs.webkit.org/attachment.cgi?id=365893&action=review




--- Comment #30 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 365893
  --> https://bugs.webkit.org/attachment.cgi?id=365893
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365893&action=review

>>>>>>> Source/WTF/wtf/FastMalloc.cpp:271
>>>>>>> +}
>>>>>> 
>>>>>> Do we really need this concept? Which OS will fail a realloc on an
aligned malloc? Is it just Windows? If so, can the Windows FastMalloc path just
add a call to _aligned_realloc?
>>>>> 
>>>>> 1. In Darwin libmalloc, it succeeds and it is guaranteed, and documented
in `man`.
>>>>> So, in Darwin, we can just return `true`, and the code is doing this.
>>>>> 
>>>>> 2. In Windows, _aligned_malloc & realloc fail. _aligned_malloc &
_aligned_realloc succeed.
>>>>> And yes. As you said, we can make
>>>>> 1) fastMalloc to call _aligned_malloc with 8 or 16
>>>>> 2) fastRealloc to call _aligned_realloc with 8 or 16
>>>>> 3) fastFree to call _aligned_free
>>>>> But not sure about the impact in terms of performance, but I think it
does not have an impact as long as we use 8 or 16 for default alignment.
>>>>> 
>>>>> 3. In Linux GLibC, it succeeds, but it is not guaranteed. It depends on
the current implementation. In the other libc, it depends on the implementation
of each Libc.
>>>>> 
>>>>> So, due to (3), anyway, we need to have this concept.
>>>> 
>>>> I see. Thanks for the explanation!
>>>> 
>>>> I wonder if it would be better to have a Windows path in FastMalloc (to
call _aligned_realloc) and libc path in the non-Darwin DebugHeap (to call
posix_memalign and memcpy). Would that work? (It's nice for client code when
FastMalloc can paper over these platform differences.)
>>> 
>>> Sounds like a good idea! If we change Windows to always using
_aligned_malloc / _aligned_realloc / _aligned_free, we can completely hide this
concept behind the FastMalloc.
>>> The problem was Windows' fastAlignedMalloc and fastAlignedFree. We cannot
call realloc and free on fastAlignedMalloc memory, and this problem leaded to
this complexity. But if we change Windows' fastMalloc / fastFree
implementations to always using _aligned_malloc etc. we can meet the following
features.
>>> 
>>> 1. fastMalloced memory can be freed by fastFree
>>> 2. fastAlignedMalloced memory can be freed by fastFree (this was not
possible, and it prevented us from introducing fallback implementation to
fastRealloc (we need to perform "free" onto the given memory, but we do not
know whether the memory is allocated from fastMalloc or fastAlignedMalloc)).
>>> 
>>> This allows us to implement fastRealloc for the non Darwin & non Windows
environments as "malloc, memcpy and free".
>> 
>> Ah! No, unfortunately :(
>> "realloc" only takes (1) a pointer to the previously allocated memory and
(2) size for newly allocated memory. But we need to know the size of (1)'s
memory region, since,
>> 
>> realloc(pointer, largerSize)
>> =>
>> 
>> void* memory = malloc(largerSize);
>> memcpy(memory, pointer, min(largerSize, sizeOfPointer));  // !!!
>> free(pointer);
>> 
>> But we do not know "sizeOfPointer" here. We can do that if we pass the
allocated memory size to "realloc" function, but it adds additional complexity,
and I think it is more complex and error prone than checking
"canUseReallocOnToAlignedMallocedPointer".
> 
> Another way is stopping using "posix_memalign" for allocating LargeAllocation
in JSC GC. Use "malloc" and adjust the pointer to align it to 16B.
> This way, we can always use "realloc" onto LargeAllocation since we know that
this is allocated by "malloc". I think it would be cleaner since,
> 
> 1. anyway, we already have a code handling misalignment for realloc-ed
memory.
> 2. we do not need to rely on this "realloc"'s platform behavior.
> 
> I'll try to do this.

I've tried, but it seems that this is also not so good than I thought. We need
to add non-aligned version of allocation-related member functions to
JSC::AlignedMemoryAllocator,