Bug 179288 – GC should support isoheaps

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 326045 [details]
it's a start

[Filip Pizlo]

Created attachment 326052 [details]
a bit more

[Filip Pizlo]

Created attachment 326076 [details]
more

[Filip Pizlo]

Created attachment 326160 [details]
close to done

[Filip Pizlo]

Created attachment 326228 [details]
almost done

Oh man, the patch size is a total lie.  That's what happens when files get renamed and then edited.

[Filip Pizlo]

Created attachment 326254 [details]
more

it's starting to compile!

[Filip Pizlo]

Created attachment 326260 [details]
jsc is compiling maybe

[Filip Pizlo]

Created attachment 326264 [details]
more things compile

[Filip Pizlo]

Created attachment 327208 [details]
it ran 3d-cube so it's probably solid

[EWS Watchlist]

Attachment 327208 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/IsoAlignedMemoryAllocator.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSSegmentedVariableObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/VM.cpp:80:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:43:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "deferralContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "failureMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/CompleteSubspace.cpp:61:  Declaration has space between type name and * in void *result  [whitespace/declaration] [3]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSDestructibleObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/JSStringHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 12 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #9)
> Created attachment 327208 [details]
> it ran 3d-cube so it's probably solid

Wow, totally called it.  JSC and WK tests passed.

[Filip Pizlo]

Created attachment 327342 [details]
the patch

[EWS Watchlist]

Attachment 327342 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/IsoAlignedMemoryAllocator.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSSegmentedVariableObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/VM.cpp:80:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:43:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "deferralContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "failureMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/CompleteSubspace.cpp:61:  Declaration has space between type name and * in void *result  [whitespace/declaration] [3]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSDestructibleObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/JSStringHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 12 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 327343 [details]
the patch

[EWS Watchlist]

Attachment 327343 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/IsoAlignedMemoryAllocator.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSSegmentedVariableObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/VM.cpp:80:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:43:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "deferralContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "failureMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/CompleteSubspace.cpp:61:  Declaration has space between type name and * in void *result  [whitespace/declaration] [3]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSDestructibleObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/JSStringHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 12 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 327344 [details]
the patch

[EWS Watchlist]

Attachment 327344 [details] did not pass style-queue:


ERROR: Source/WebCore/ForwardingHeaders/runtime/JSSegmentedVariableObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/heap/IsoAlignedMemoryAllocator.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:43:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "deferralContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "failureMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSDestructibleObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/JSStringHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/VM.cpp:80:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/CompleteSubspace.cpp:61:  Declaration has space between type name and * in void *result  [whitespace/declaration] [3]
Total errors found: 12 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 327580 [details]
the patch

[EWS Watchlist]

Attachment 327580 [details] did not pass style-queue:


ERROR: Source/WebCore/ForwardingHeaders/runtime/JSSegmentedVariableObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/heap/IsoAlignedMemoryAllocator.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:37:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:43:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "deferralContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/IsoSubspace.h:46:  The parameter name "failureMode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/ForwardingHeaders/runtime/JSDestructibleObjectHeapCellType.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/JavaScriptCore/runtime/JSStringHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/VM.cpp:80:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectHeapCellType.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/CompleteSubspace.cpp:61:  Declaration has space between type name and * in void *result  [whitespace/declaration] [3]
Total errors found: 12 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 327580 [details]
the patch

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

r=me
My main comment is about your use of AllocationFailureMode inside allocatorFor and allocatorForNonVirtual. It feels like a weird use since it can return nullptr.

> Source/JavaScriptCore/heap/CompleteSubspace.cpp:83
> +    // enough: it will have 

unfinished thought here (I know this is a copied comment, but we might as well clean it up now)

> Source/JavaScriptCore/heap/CompleteSubspace.cpp:126
> +    if (MarkedAllocator* allocator = allocatorFor(size, AllocationFailureMode::Assert))
> +        return allocator->allocate(deferralContext, AllocationFailureMode::ReturnNull);

Ditto about weird use for AllocationFailureMode

> Source/JavaScriptCore/heap/CompleteSubspace.h:61
> +ALWAYS_INLINE MarkedAllocator* CompleteSubspace::allocatorForNonVirtual(size_t size, AllocationFailureMode failureMode)

ditto about weird use of AllocationFailureMode

> Source/JavaScriptCore/heap/HeapCellType.cpp:47
> +        MethodTable::DestroyFunctionPtr destroy = classInfo->methodTable.destroy;

style nit: I'd just write `auto` for the type here

> Source/JavaScriptCore/heap/Subspace.h:58
> +    virtual MarkedAllocator* allocatorFor(size_t, AllocationFailureMode) = 0;

This feels like a really weird use of AllocationFailureMode. We never really assert if it's null in the subclasses of Subspace. This is allowed to return nullptr even if AllocationFailurueMode == Assert.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1575
> +        MarkedAllocator* allocator = subspaceFor<ClassType>(vm)->allocatorForNonVirtual(size, AllocationFailureMode::Assert);

See comments about AllocationFailureMode used for this function and the virtual variant. I think we need some other type because this can return nullptr even though it's called with Assert

> Source/JavaScriptCore/runtime/ExecutableBase.h:87
> +    static void subspaceFor(VM&) { }

Maybe also RELEASE_ASSERT_NOT_REACHED? (I get what you're doing w/ return type, but might as well catch anyone calling this w/out caring for return type)

Alternatively, does =delete work on arbitrary member functions? If so, you could just do that too I believe and the compiler will catch it in all scenarios.

[Filip Pizlo]

(In reply to Saam Barati from comment #20)
> Comment on attachment 327580 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327580&action=review
> 
> r=me
> My main comment is about your use of AllocationFailureMode inside
> allocatorFor and allocatorForNonVirtual. It feels like a weird use since it
> can return nullptr.
> 
> > Source/JavaScriptCore/heap/CompleteSubspace.cpp:83
> > +    // enough: it will have 
> 
> unfinished thought here (I know this is a copied comment, but we might as
> well clean it up now)
> 
> > Source/JavaScriptCore/heap/CompleteSubspace.cpp:126
> > +    if (MarkedAllocator* allocator = allocatorFor(size, AllocationFailureMode::Assert))
> > +        return allocator->allocate(deferralContext, AllocationFailureMode::ReturnNull);
> 
> Ditto about weird use for AllocationFailureMode
> 
> > Source/JavaScriptCore/heap/CompleteSubspace.h:61
> > +ALWAYS_INLINE MarkedAllocator* CompleteSubspace::allocatorForNonVirtual(size_t size, AllocationFailureMode failureMode)
> 
> ditto about weird use of AllocationFailureMode
> 
> > Source/JavaScriptCore/heap/HeapCellType.cpp:47
> > +        MethodTable::DestroyFunctionPtr destroy = classInfo->methodTable.destroy;
> 
> style nit: I'd just write `auto` for the type here
> 
> > Source/JavaScriptCore/heap/Subspace.h:58
> > +    virtual MarkedAllocator* allocatorFor(size_t, AllocationFailureMode) = 0;
> 
> This feels like a really weird use of AllocationFailureMode. We never really
> assert if it's null in the subclasses of Subspace. This is allowed to return
> nullptr even if AllocationFailurueMode == Assert.
> 
> > Source/JavaScriptCore/jit/AssemblyHelpers.h:1575
> > +        MarkedAllocator* allocator = subspaceFor<ClassType>(vm)->allocatorForNonVirtual(size, AllocationFailureMode::Assert);
> 
> See comments about AllocationFailureMode used for this function and the
> virtual variant. I think we need some other type because this can return
> nullptr even though it's called with Assert

Right, for large allocations maybe. I’ll look into giving that a different enum. 

> 
> > Source/JavaScriptCore/runtime/ExecutableBase.h:87
> > +    static void subspaceFor(VM&) { }
> 
> Maybe also RELEASE_ASSERT_NOT_REACHED? (I get what you're doing w/ return
> type, but might as well catch anyone calling this w/out caring for return
> type)
> 
> Alternatively, does =delete work on arbitrary member functions? If so, you
> could just do that too I believe and the compiler will catch it in all
> scenarios.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #21)
> (In reply to Saam Barati from comment #20)
> > Comment on attachment 327580 [details]
> > the patch
> > 
> > > Source/JavaScriptCore/jit/AssemblyHelpers.h:1575
> > > +        MarkedAllocator* allocator = subspaceFor<ClassType>(vm)->allocatorForNonVirtual(size, AllocationFailureMode::Assert);
> > 
> > See comments about AllocationFailureMode used for this function and the
> > virtual variant. I think we need some other type because this can return
> > nullptr even though it's called with Assert
> 
> Right, for large allocations maybe. I’ll look into giving that a different
> enum. 

Yeah, that was all messed up. I introduced:

enum class AllocatorForMode {
    MustAlreadyHaveAllocator,
    EnsureAllocator,
    AllocatorIfExists
};

Now it makes more sense.

[Filip Pizlo]

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

[Radar WebKit Bug Importer]

<rdar://problem/35765225>

[Yusuke Suzuki]

Comment on attachment 327580 [details]
the patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5709
> +        MarkedAllocator* allocator = subspaceFor<JSRopeString>(vm())->allocatorForNonVirtual(sizeof(JSRopeString), AllocationFailureMode::Assert);

This crashes in Linux ports. But I don't think this is limited in Linux environment.
The reason is simple: an allocator for sizeof(JSRopeString)'s size class is not initialized because JSRopeString is not allocated well in initialization phase.

[Alexey Proskuryakov]

This change added a lot of leaks on the bots (from ~400 Kb to over 100 Mb).

[Filip Pizlo]

(In reply to Alexey Proskuryakov from comment #26)
> This change added a lot of leaks on the bots (from ~400 Kb to over 100 Mb).

Bug?  Happy to take a look.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #27)
> (In reply to Alexey Proskuryakov from comment #26)
> > This change added a lot of leaks on the bots (from ~400 Kb to over 100 Mb).
> 
> Bug?  Happy to take a look.

Found it.  https://bugs.webkit.org/show_bug.cgi?id=180425

[Keith Miller]

Comment on attachment 327580 [details]
the patch

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

> Source/JavaScriptCore/heap/CompleteSubspace.cpp:81
> +    // prevent simultaneously MarkedAllocator creations from multiple threads. This code ensures

Typo: simultaneously => simultaneous.

>>> Source/JavaScriptCore/heap/CompleteSubspace.cpp:83
>>> +    // enough: it will have 
>> 
>> unfinished thought here (I know this is a copied comment, but we might as well clean it up now)
> 
> Right, for large allocations maybe. I’ll look into giving that a different enum.

You never cleaned this up! :(