...
<rdar://problem/36472514>
Created attachment 331267 [details] it's a start
Created attachment 331293 [details] a bit more
Created attachment 331308 [details] moving more stuff into LocalAllocator
Created attachment 331310 [details] maybe it's written
Attachment 331310 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1496: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1537: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:67: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:53: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331344 [details] getting more things to compile
Created attachment 331352 [details] more
Created attachment 331796 [details] got it to build! wow!
Attachment 331796 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1497: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1499: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1537: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:68: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:58: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331821 [details] TLCs can say hello!
Attachment 331821 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1497: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1499: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1537: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:68: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:58: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331846 [details] can run significant things
Created attachment 331848 [details] passes JSC tests! Yay!
Created attachment 331850 [details] rebased
Attachment 331850 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1500: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1534: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:72: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:59: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331851 [details] the patch
Attachment 331851 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1500: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1534: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:77: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:59: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331853 [details] the patch
Attachment 331853 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1500: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1534: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:77: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:59: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331959 [details] the patch
Attachment 331959 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1500: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1534: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:77: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:59: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 331961 [details] the patch
Attachment 331961 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1498: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1500: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:1534: The parameter name "subspace" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/ThreadLocalCache.h:77: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/VM.h:57: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.cpp:589: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/LocalAllocator.h:59: The parameter name "failureMode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/Subspace.h:30: Alphabetical sorting problem. [build/include_order] [4] WARNING: This machine could support 4 simulators, but is only configured for 3. WARNING: Please see <https://trac.webkit.org/wiki/IncreasingKernelLimits>. Total errors found: 8 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 331961 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=331961&action=review r=me with some feedback. > Source/JavaScriptCore/ChangeLog:12 > + This is a big step towards object distancing by site origin. This patch implements TLCs, or > + thread-local caches, which allow each thread to allocate from its own free lists. It also > + means that any given thread can context-switch TLCs. This will allow us to do separate > + allocation for separate site origins. Eventually, once we reshape how MarkedBlock looks, this > + will allow us to have a hard distancing constraint between objects from different origins. I think it is worth adding additional documentation on the architecture (how allocations now work, and all the invariants in the system). For example, All allocations are now done via a Allocator instead of directly via the BlockDirectory. There is 1 ThreadLocalCache per VM: there's a 1:1 relationship between VM, heap, and the ThreadLocalCache instance. The ThreadLocalCache may outlive the VM and the heap, because it may be installed on threads that have used its VM. If the VM is dead, the ThreadLocalCache will be destroyed when all threads (that had it installed on them) have died. relationship between Allocator and BlockDirectory. relationship between LocalAllocator and BlockDirectory. relationship between BlockDirectory and ThreadLocalCache. etc. It took me a while to grok the relationships between all the actors in the system and what their roles are. And I was only able to do so by reading the many lines of code in this patch (which took a long time). Even now, at the end of my reviewing this patch, I'm already starting to get hazy on some these relationships. For anyone else coming to read parts of this code later and trying to understand what's what, it would be easy to be lost without a high level guide on the architecture. It would be very helpful if one can simply read a short summary of the architecture in one place instead of having to grok thru many pages of code in disparate places and having to reverse engineer their intent. So, can you add an architecture summary here in the ChangeLog? Or perhaps also copy it into a comment blob in ThreadLocalCache.h/cpp where it'll be easier to find. Thanks. > Source/JavaScriptCore/heap/LocalAllocator.cpp:195 > + // for for using TLC allocation from JIT threads. duplicate "for" > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:43 > + m_data = allocateData(); nit: I recommend calling an ensureData() instead (see below for more discussion). > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:51 > +ThreadLocalCache::Data* ThreadLocalCache::allocateData() nit: I recommend changing this into a void ensureData() method, and having it destroy the old m_data if an old one exists. That brings the groups the lifecycle of Data into ensureData() and destroyData() methods instead of having parts in ThreadLocalCache::allocatorSlow() as well. > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:106 > + Data* oldData = m_data; > + m_data = allocateData(); > + destroyData(oldData); nit: I recommend doing all this inside an ensureData() method instead. > Source/JavaScriptCore/heap/ThreadLocalCache.h:60 > + static ptrdiff_t offsetOfSize() { return OBJECT_OFFSETOF(Data, size); } > + static ptrdiff_t offsetOfFirstAllocator() { return OBJECT_OFFSETOF(Data, allocator); } nit: It would be more accurate to name these offsetOfDataSize() and offsetOfDataFirstAllocator(). > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:45 > + if (data) nit: use LIKELY() around (data)? > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:52 > + if (getImpl(vm) == m_data) nit: use LIKELY() around condition? > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.cpp:1 > +/* malm DONE > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.h:63 > + size_t m_size { 0 }; > + Vector<BlockDirectory*> m_directories; I was going to to comment that m_size if redundant because it can be implied / computed from m_directories.size(). Ditto for size in Snapshot. However, after groking the entire patch, I see that m_size being in units of bytes instead of an index has a small advantage in code emitted for variable JITAllocators. Otherwise, we can just set offset to the vector index instead of an increment of sizeof(LocalAllocator). On a similar note, I was also going to suggest that we can reserve offset 0 to be the null allocator instead of using UINT_MAX. That has some benefits and can simplify a lot of code, and make it more intuitive. It would mean that offset is now defined in terms of vector index + 1, instead of just the vector index (or its bytes size equivalent). That said, I don't think it's worth making the change for now. We can always refactor this code later to apply that change if desired. > Source/JavaScriptCore/jit/AssemblyHelpers.h:1493 > - > + Please undo. > Source/JavaScriptCore/runtime/VM.cpp:390 > - > + please undo.
(In reply to Mark Lam from comment #25) > > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.cpp:1 > > +/* > > malm DONE Ignore this. This was a typo in one of my place markers, and hence why I neglected to remove it before finalizing the review.
Comment on attachment 331961 [details] the patch The SomeRegisterWithClobber ValueRep LGTM
(In reply to Mark Lam from comment #25) > Comment on attachment 331961 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331961&action=review > > r=me with some feedback. > > > Source/JavaScriptCore/ChangeLog:12 > > + This is a big step towards object distancing by site origin. This patch implements TLCs, or > > + thread-local caches, which allow each thread to allocate from its own free lists. It also > > + means that any given thread can context-switch TLCs. This will allow us to do separate > > + allocation for separate site origins. Eventually, once we reshape how MarkedBlock looks, this > > + will allow us to have a hard distancing constraint between objects from different origins. > > I think it is worth adding additional documentation on the architecture (how > allocations now work, and all the invariants in the system). For example, > > All allocations are now done via a Allocator instead of directly via the > BlockDirectory. > There is 1 ThreadLocalCache per VM: there's a 1:1 relationship between > VM, heap, and the ThreadLocalCache instance. > The ThreadLocalCache may outlive the VM and the heap, because it may be > installed on threads that have used its VM. If the VM is dead, the > ThreadLocalCache will be destroyed when all threads (that had it installed > on them) have died. > > relationship between Allocator and BlockDirectory. > relationship between LocalAllocator and BlockDirectory. > relationship between BlockDirectory and ThreadLocalCache. > etc. > > It took me a while to grok the relationships between all the actors in the > system and what their roles are. And I was only able to do so by reading > the many lines of code in this patch (which took a long time). Even now, at > the end of my reviewing this patch, I'm already starting to get hazy on some > these relationships. For anyone else coming to read parts of this code > later and trying to understand what's what, it would be easy to be lost > without a high level guide on the architecture. It would be very helpful if > one can simply read a short summary of the architecture in one place instead > of having to grok thru many pages of code in disparate places and having to > reverse engineer their intent. > > So, can you add an architecture summary here in the ChangeLog? Or perhaps > also copy it into a comment blob in ThreadLocalCache.h/cpp where it'll be > easier to find. Thanks. Fixed. > > > Source/JavaScriptCore/heap/LocalAllocator.cpp:195 > > + // for for using TLC allocation from JIT threads. > > duplicate "for" Fixed > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:43 > > + m_data = allocateData(); > > nit: I recommend calling an ensureData() instead (see below for more > discussion). Good idea. I'll do that. > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:51 > > +ThreadLocalCache::Data* ThreadLocalCache::allocateData() > > nit: I recommend changing this into a void ensureData() method, and having > it destroy the old m_data if an old one exists. That brings the groups the > lifecycle of Data into ensureData() and destroyData() methods instead of > having parts in ThreadLocalCache::allocatorSlow() as well. > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:106 > > + Data* oldData = m_data; > > + m_data = allocateData(); > > + destroyData(oldData); > > nit: I recommend doing all this inside an ensureData() method instead. I replaced allocateData() with ensureData(). allocatorSlow() is different because it also returns an allocator (the constructor does not want that). destroyData() is needed by both ensureData() and the destructor. > > > Source/JavaScriptCore/heap/ThreadLocalCache.h:60 > > + static ptrdiff_t offsetOfSize() { return OBJECT_OFFSETOF(Data, size); } > > + static ptrdiff_t offsetOfFirstAllocator() { return OBJECT_OFFSETOF(Data, allocator); } > > nit: It would be more accurate to name these offsetOfDataSize() and > offsetOfDataFirstAllocator(). I changed them to offsetOfSizeInData() and offsetOfFirstAllocatorInData(). > > > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:45 > > + if (data) > > nit: use LIKELY() around (data)? OK. > > > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:52 > > + if (getImpl(vm) == m_data) > > nit: use LIKELY() around condition? I don't actually know if that's likely. > > > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.cpp:1 > > +/* > > malm DONE > > > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.h:63 > > + size_t m_size { 0 }; > > + Vector<BlockDirectory*> m_directories; > > I was going to to comment that m_size if redundant because it can be implied > / computed from m_directories.size(). Ditto for size in Snapshot. However, > after groking the entire patch, I see that m_size being in units of bytes > instead of an index has a small advantage in code emitted for variable > JITAllocators. Otherwise, we can just set offset to the vector index > instead of an increment of sizeof(LocalAllocator). Not just the JIT. Every allocation first gets an Allocator (currently an offset) and adds it to the ThreadLocalData::Data. If it was an index then this would be an imul because LocalAllocator is likely to have a not-power-of-2 size. > > On a similar note, I was also going to suggest that we can reserve offset 0 > to be the null allocator instead of using UINT_MAX. That has some benefits > and can simplify a lot of code, and make it more intuitive. It would mean > that offset is now defined in terms of vector index + 1, instead of just the > vector index (or its bytes size equivalent). Currently we use the fact that bad allocator is UINT_MAX to skip one check: if the CompleteSubspace returns a null allocator then it is trivially out of bounds. Again, this saves one math op. > > That said, I don't think it's worth making the change for now. We can > always refactor this code later to apply that change if desired. > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:1493 > > - > > + > > Please undo. Fixed. > > > Source/JavaScriptCore/runtime/VM.cpp:390 > > - > > + > > please undo. Fixed.
(In reply to Filip Pizlo from comment #28) > (In reply to Mark Lam from comment #25) > > Comment on attachment 331961 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=331961&action=review > > > > r=me with some feedback. > > > > > Source/JavaScriptCore/ChangeLog:12 > > > + This is a big step towards object distancing by site origin. This patch implements TLCs, or > > > + thread-local caches, which allow each thread to allocate from its own free lists. It also > > > + means that any given thread can context-switch TLCs. This will allow us to do separate > > > + allocation for separate site origins. Eventually, once we reshape how MarkedBlock looks, this > > > + will allow us to have a hard distancing constraint between objects from different origins. > > > > I think it is worth adding additional documentation on the architecture (how > > allocations now work, and all the invariants in the system). For example, > > > > All allocations are now done via a Allocator instead of directly via the > > BlockDirectory. > > There is 1 ThreadLocalCache per VM: there's a 1:1 relationship between > > VM, heap, and the ThreadLocalCache instance. > > The ThreadLocalCache may outlive the VM and the heap, because it may be > > installed on threads that have used its VM. If the VM is dead, the > > ThreadLocalCache will be destroyed when all threads (that had it installed > > on them) have died. > > > > relationship between Allocator and BlockDirectory. > > relationship between LocalAllocator and BlockDirectory. > > relationship between BlockDirectory and ThreadLocalCache. > > etc. > > > > It took me a while to grok the relationships between all the actors in the > > system and what their roles are. And I was only able to do so by reading > > the many lines of code in this patch (which took a long time). Even now, at > > the end of my reviewing this patch, I'm already starting to get hazy on some > > these relationships. For anyone else coming to read parts of this code > > later and trying to understand what's what, it would be easy to be lost > > without a high level guide on the architecture. It would be very helpful if > > one can simply read a short summary of the architecture in one place instead > > of having to grok thru many pages of code in disparate places and having to > > reverse engineer their intent. > > > > So, can you add an architecture summary here in the ChangeLog? Or perhaps > > also copy it into a comment blob in ThreadLocalCache.h/cpp where it'll be > > easier to find. Thanks. > > Fixed. > > > > > > Source/JavaScriptCore/heap/LocalAllocator.cpp:195 > > > + // for for using TLC allocation from JIT threads. > > > > duplicate "for" > > Fixed > > > > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:43 > > > + m_data = allocateData(); > > > > nit: I recommend calling an ensureData() instead (see below for more > > discussion). > > Good idea. I'll do that. Sorry, this isn't a valid change. allocateData() must not install data. Therefore, I think it makes sense if allocateData() also doesn't destroy data. > > > > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:51 > > > +ThreadLocalCache::Data* ThreadLocalCache::allocateData() > > > > nit: I recommend changing this into a void ensureData() method, and having > > it destroy the old m_data if an old one exists. That brings the groups the > > lifecycle of Data into ensureData() and destroyData() methods instead of > > having parts in ThreadLocalCache::allocatorSlow() as well. > > > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:106 > > > + Data* oldData = m_data; > > > + m_data = allocateData(); > > > + destroyData(oldData); > > > > nit: I recommend doing all this inside an ensureData() method instead. > > I replaced allocateData() with ensureData(). allocatorSlow() is different > because it also returns an allocator (the constructor does not want that). > destroyData() is needed by both ensureData() and the destructor. > > > > > > Source/JavaScriptCore/heap/ThreadLocalCache.h:60 > > > + static ptrdiff_t offsetOfSize() { return OBJECT_OFFSETOF(Data, size); } > > > + static ptrdiff_t offsetOfFirstAllocator() { return OBJECT_OFFSETOF(Data, allocator); } > > > > nit: It would be more accurate to name these offsetOfDataSize() and > > offsetOfDataFirstAllocator(). > > I changed them to offsetOfSizeInData() and offsetOfFirstAllocatorInData(). > > > > > > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:45 > > > + if (data) > > > > nit: use LIKELY() around (data)? > > OK. > > > > > > Source/JavaScriptCore/heap/ThreadLocalCacheInlines.h:52 > > > + if (getImpl(vm) == m_data) > > > > nit: use LIKELY() around condition? > > I don't actually know if that's likely. > > > > > > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.cpp:1 > > > +/* > > > > malm DONE > > > > > Source/JavaScriptCore/heap/ThreadLocalCacheLayout.h:63 > > > + size_t m_size { 0 }; > > > + Vector<BlockDirectory*> m_directories; > > > > I was going to to comment that m_size if redundant because it can be implied > > / computed from m_directories.size(). Ditto for size in Snapshot. However, > > after groking the entire patch, I see that m_size being in units of bytes > > instead of an index has a small advantage in code emitted for variable > > JITAllocators. Otherwise, we can just set offset to the vector index > > instead of an increment of sizeof(LocalAllocator). > > Not just the JIT. Every allocation first gets an Allocator (currently an > offset) and adds it to the ThreadLocalData::Data. > > If it was an index then this would be an imul because LocalAllocator is > likely to have a not-power-of-2 size. > > > > > On a similar note, I was also going to suggest that we can reserve offset 0 > > to be the null allocator instead of using UINT_MAX. That has some benefits > > and can simplify a lot of code, and make it more intuitive. It would mean > > that offset is now defined in terms of vector index + 1, instead of just the > > vector index (or its bytes size equivalent). > > Currently we use the fact that bad allocator is UINT_MAX to skip one check: > if the CompleteSubspace returns a null allocator then it is trivially out of > bounds. > > Again, this saves one math op. > > > > > That said, I don't think it's worth making the change for now. We can > > always refactor this code later to apply that change if desired. > > > > > Source/JavaScriptCore/jit/AssemblyHelpers.h:1493 > > > - > > > + > > > > Please undo. > > Fixed. > > > > > > Source/JavaScriptCore/runtime/VM.cpp:390 > > > - > > > + > > > > please undo. > > Fixed.
Landed in https://trac.webkit.org/changeset/227592/webkit
(In reply to Filip Pizlo from comment #30) > Landed in https://trac.webkit.org/changeset/227592/webkit FYI, it made zillion tests crash on AArch64 Linux. cc-ing Igalians, maybe you are interested in this bug. before: - 16 fails - https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/2881 after: - 8136 fails - https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/2882
(In reply to Csaba Osztrogonác from comment #31) > FYI, it made zillion tests crash on AArch64 Linux. > cc-ing Igalians, maybe you are interested in this bug. > > before: > - 16 fails > - > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/ > 2881 > > after: > - 8136 fails > - > https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/ > 2882 Thanks for the heads-up, we are indeed....
This also made 4000 iOS JSC tests fail
(In reply to Saam Barati from comment #33) > This also made 4000 iOS JSC tests fail OK, I'll roll it out for now.
Re-opened since this is blocked by bug 182110
Relanded with ARM64 fix in https://trac.webkit.org/changeset/227617/webkit
(In reply to Filip Pizlo from comment #36) > Relanded with ARM64 fix in https://trac.webkit.org/changeset/227617/webkit It seems it didn't fix the AArch64 Linux issue. before: - 19 fails - https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/2888 after: - 8879 fails - https://build.webkit.org/builders/JSCOnly%20Linux%20AArch64%20Release/builds/2889