Bug 181559 – JSC GC should support TLCs (thread local caches)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Radar WebKit Bug Importer]

<rdar://problem/36472514>

[Filip Pizlo]

Created attachment 331267 [details]
it's a start

[Filip Pizlo]

Created attachment 331293 [details]
a bit more

[Filip Pizlo]

Created attachment 331308 [details]
moving more stuff into LocalAllocator

[Filip Pizlo]

Created attachment 331310 [details]
maybe it's written

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331344 [details]
getting more things to compile

[Filip Pizlo]

Created attachment 331352 [details]
more

[Filip Pizlo]

Created attachment 331796 [details]
got it to build!

wow!

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331821 [details]
TLCs can say hello!

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331846 [details]
can run significant things

[Filip Pizlo]

Created attachment 331848 [details]
passes JSC tests!

Yay!

[Filip Pizlo]

Created attachment 331850 [details]
rebased

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331851 [details]
the patch

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331853 [details]
the patch

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331959 [details]
the patch

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 331961 [details]
the patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Mark Lam]

(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.

[Saam Barati]

Comment on attachment 331961 [details]
the patch

The SomeRegisterWithClobber ValueRep LGTM

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/227592/webkit

[Csaba Osztrogonác]

(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

[Michael Catanzaro]

(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....

[Saam Barati]

This also made 4000 iOS JSC tests fail

[Filip Pizlo]

(In reply to Saam Barati from comment #33)
> This also made 4000 iOS JSC tests fail

OK, I'll roll it out for now.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 182110

[Filip Pizlo]

Relanded with ARM64 fix in https://trac.webkit.org/changeset/227617/webkit

[Csaba Osztrogonác]

(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