MetaAllocator tries to share one page for assemblies. Currently all threads share one MetaAllocator. When one thread is using a page to execute an assembly, another thread may want to write on it. Making the allocator thread-specific for WX_EXCLUSIVE is a quick solution.
Created attachment 130679 [details] the patch
Attachment 130679 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/jit/ExecutableAllocator.cpp:107: g_allocator is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/JavaScriptCore/jit/ExecutableAllocator.cpp:122: g_allocator is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130679 [details] the patch Attachment 130679 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11855264
Comment on attachment 130679 [details] the patch Attachment 130679 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11853279
Comment on attachment 130679 [details] the patch Attachment 130679 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11854270
Comment on attachment 130679 [details] the patch Attachment 130679 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11849377
Created attachment 130710 [details] the correct patch The previous patch was actually an obsolete one that never worked
Darn... Some implementations of TCMalloc_SpinLock has Finalize, and some don't... Can we just use Mutex defined in ThreadingPrimitives.h?
Comment on attachment 130710 [details] the correct patch Attachment 130710 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11850444
Comment on attachment 130710 [details] the correct patch This would break JavaScriptCore. A JSC VM instance may be used from multiple threads.
(In reply to comment #10) > (From update of attachment 130710 [details]) > This would break JavaScriptCore. > > A JSC VM instance may be used from multiple threads. Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE? Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)?
Phil, I guess fixing this would mean splitting the ExecutableAllocator to be tiered into a have a locked, global page allocation, and a per-global-data sub page allocation pool. What do you think? G.
(In reply to comment #12) > Phil, > > I guess fixing this would mean splitting the ExecutableAllocator to be tiered into a have a locked, global page allocation, and a per-global-data sub page allocation pool. What do you think? > > G. I tend to think that WX_EXCLUSIVE should just be thread safe. Is there any reason why it cannot be? The MetaAllocator has a simple thread safety contract: it protects itself with a lock. Hence once its code runs, we know that we're serialized. If WX_EXCLUSIVE needs to do additional things, it should also protect itself with a lock - either the MetaAllocator's lock or a different one. Probably doesn't matter. I don't buy that we should be splitting the executable memory allocator into separate memory allocators. This just increases fragmentation, which is what we are trying to avoid at all costs.
(In reply to comment #0) > MetaAllocator tries to share one page for assemblies. Currently all threads share one MetaAllocator. When one thread is using a page to execute an assembly, another thread may want to write on it. > > Making the allocator thread-specific for WX_EXCLUSIVE is a quick solution. I think for this code I would prefer that we do the right solution, rather than the quick one, though see my comment below.
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 130710 [details] [details]) > > This would break JavaScriptCore. > > > > A JSC VM instance may be used from multiple threads. > > Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE? > > Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)? I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe.
(In reply to comment #15) > (In reply to comment #11) > > (In reply to comment #10) > > > (From update of attachment 130710 [details] [details] [details]) > > > This would break JavaScriptCore. > > > > > > A JSC VM instance may be used from multiple threads. > > > > Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE? > > > > Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)? > > I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe. Was WX_EXCLUSIVE ever thread-safe? 1) If not, then wrapping with another #define seems not necessary. 2) If yes, I'm thinking we can make every global object own a MetaAllocator. So the global object is still thread-safe, and there is no conflict from web worker. Do you think this is right thing to do?
Created attachment 131071 [details] For WX_EXCLUSIVE, let each global object own a meta allocator
Created attachment 131074 [details] One MetaAllocator per JS global object (only for WX_EXCLUSIVE) Had to add a dtor and put it to the cpp file otherwise OwnPtr doesn't know how to delete DemandExecutableAllocator.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #11) > > > (In reply to comment #10) > > > > (From update of attachment 130710 [details] [details] [details] [details]) > > > > This would break JavaScriptCore. > > > > > > > > A JSC VM instance may be used from multiple threads. > > > > > > Then any suggestion about how to recover the ASSEMBLER_WX_EXCLUSIVE? > > > > > > Can I wrap the code with something like #if ENABLE(ASSEMBLER_WX_EXCLUSIVE) && USE(JSC_SINGLE_THREADED)? > > > > I would be OK with this, but please open a bug about making WX_EXCLUSIVE thread safe. > > Was WX_EXCLUSIVE ever thread-safe? Yes, because the old allocator would put each piece of code onto a separate page. > > 1) If not, then wrapping with another #define seems not necessary. > 2) If yes, I'm thinking we can make every global object own a MetaAllocator. So the global object is still thread-safe, and there is no conflict from web worker. Do you think this is right thing to do? I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE. That seems so much easier and will result in a smaller patch. Can you explain why you did not do it that way?
Created attachment 131080 [details] fix build error for ExecutableAllocatorFixedVMPool
(In reply to comment #19) > I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE. That seems so much easier and will result in a smaller patch. Can you explain why you did not do it that way? Is this the whole problem? I was thinking we had a scenario where: Thread A is allocated space on page X. Thread A makes page X writable. Thread A generates code on page X. Thread A makes page X executable. Thread A starts running code on page X. Thread B is allocated space on page X. Thread B makes page X writable. Thread A crashes. Just locking the region in with the writable/executable permissions are changed doesn't seem to be enough, you'd need to interrupt other running threads, wouldn't you?
(In reply to comment #21) > (In reply to comment #19) > > I don't understand why you think this is an easier solution than putting a lock around methods that do the work of WX_EXCLUSIVE. That seems so much easier and will result in a smaller patch. Can you explain why you did not do it that way? > > Is this the whole problem? I was thinking we had a scenario where: > > Thread A is allocated space on page X. > Thread A makes page X writable. > Thread A generates code on page X. > Thread A makes page X executable. > Thread A starts running code on page X. > Thread B is allocated space on page X. > Thread B makes page X writable. > Thread A crashes. > > Just locking the region in with the writable/executable permissions are changed doesn't seem to be enough, you'd need to interrupt other running threads, wouldn't you? Ahh! Good point. I forgot that WX_EXCLUSIVE meant flipping pages from exec-only to write-only.
Comment on attachment 131080 [details] fix build error for ExecutableAllocatorFixedVMPool Clearing flags on attachment: 131080 Committed r110379: <http://trac.webkit.org/changeset/110379>
All reviewed patches have been landed. Closing bug.