[webkit-reviews] review canceled: [Bug 169819] [JSC] MachineThreads does not consider situation that one thread has multiple VMs : [Attachment 305030] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 23 06:04:31 PDT 2017


Yusuke Suzuki <utatane.tea at gmail.com> has canceled Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 169819: [JSC] MachineThreads does not consider situation that one thread
has multiple VMs
https://bugs.webkit.org/show_bug.cgi?id=169819

Attachment 305030: Patch

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




--- Comment #11 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 305030
  --> https://bugs.webkit.org/attachment.cgi?id=305030
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +	    It actually causes deadlock in GTK port DatabaseProcess.
> 
> I think I understand what the issue is now.  It's not so much that every VM
allocates its own MachineThreads::Thread per thread it runs on, but that the
Linux implementation of thread suspend/resume relies on having a thread
specific singleton that houses the semaphore needed to communicate with the
originating thread that requested the suspend/resume.  As a result, when
there's potentially more than one VM running on the same thread,
MachineThreads::Thread cannot serve as that thread specific singleton that the
Linux port needs.  Hence, you're factoring out the ThreadData part as the
thread specific singleton.  Did I understand the issue correctly?

Right.

>> Source/JavaScriptCore/heap/MachineStackMarker.cpp:207
>> +	return threadData.get();
> 
> I'm not too clear on the interaction of NeverDestroyed with ThreadSpecific
here.  My concern is whether this will leak memory for every thread that is
started and ended.  For example, let's say you have the following scenario:
> 
> 1. Thread T1 creates a VM.
> 2. Thread T2 enters the VM, thereby creating a ThreadSpecific<ThreadData> for
T2.
> 3. Thread T2 exits the VM, and terminates itself.
> 4. Thread T3 now enters the VM, and creates a ThreadSpecific<ThreadData> for
T3.
> 5. Thread T3 exits the VM, and terminated itself.
> 6. Repeat steps 2 and 3 for thread T4, T5, etc. just like we did for thread
T3 in steps 4 and 5.
> 
> On thread destruction, I know that ThreadSpecific will take care of
destructing ThreadData and freeing its memory.
> What I suspect is that NeverDestroyed will allocated a memory for
ThreadSpecific, but not free it.  As a result, for every thread that is created
and dies, we'll end up leaking sizeof(ThreadSpecific).
> 
> Can you test and confirm that we won't be leaking memory here?

Oooooops! Nice catch. Right, NeverDestroyed will not call the destructor.
I'll offer another way to allocate thread local storage for that, this should
be something like PerThread in bmalloc.

>> Source/JavaScriptCore/heap/MachineStackMarker.h:67
>> +	class ThreadData {
> 
> This needs to be WTF_MAKE_FAST_ALLOCATED because ThreadSpecific::destroy()
will call fastFree() on it.

Thanks! Fixed.

>> Source/JavaScriptCore/heap/MachineStackMarker.h:147
>> +	    }
> 
> nit: since these only have a single statement in their body, you can make
them all single line just like the other methods on Thread.

OK, fixed.


More information about the webkit-reviews mailing list