Bug 179288

Summary: GC should support isoheaps
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, cdumez, cmarcelo, dbates, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180185
https://bugs.webkit.org/show_bug.cgi?id=180425
Bug Depends on:    
Bug Blocks: 179876    
Attachments:
Description Flags
it's a start
none
a bit more
none
more
none
close to done
none
almost done
none
more
none
jsc is compiling maybe
none
more things compile
none
it ran 3d-cube so it's probably solid
none
the patch
none
the patch
none
the patch
none
the patch saam: review+

Description Filip Pizlo 2017-11-04 14:41:42 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-11-04 14:42:12 PDT
Created attachment 326045 [details]
it's a start
Comment 2 Filip Pizlo 2017-11-04 16:33:33 PDT
Created attachment 326052 [details]
a bit more
Comment 3 Filip Pizlo 2017-11-05 13:07:32 PST
Created attachment 326076 [details]
more
Comment 4 Filip Pizlo 2017-11-06 14:56:30 PST
Created attachment 326160 [details]
close to done
Comment 5 Filip Pizlo 2017-11-07 11:34:23 PST
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.
Comment 6 Filip Pizlo 2017-11-07 13:37:35 PST
Created attachment 326254 [details]
more

it's starting to compile!
Comment 7 Filip Pizlo 2017-11-07 14:21:36 PST
Created attachment 326260 [details]
jsc is compiling maybe
Comment 8 Filip Pizlo 2017-11-07 14:38:35 PST
Created attachment 326264 [details]
more things compile
Comment 9 Filip Pizlo 2017-11-17 11:59:32 PST
Created attachment 327208 [details]
it ran 3d-cube so it's probably solid
Comment 10 EWS Watchlist 2017-11-17 12:02:42 PST
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.
Comment 11 Filip Pizlo 2017-11-19 08:07:26 PST
(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.
Comment 12 Filip Pizlo 2017-11-19 08:10:18 PST
Created attachment 327342 [details]
the patch
Comment 13 EWS Watchlist 2017-11-19 08:12:50 PST
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.
Comment 14 Filip Pizlo 2017-11-19 08:12:55 PST
Created attachment 327343 [details]
the patch
Comment 15 EWS Watchlist 2017-11-19 08:16:30 PST
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.
Comment 16 Filip Pizlo 2017-11-19 09:15:16 PST
Created attachment 327344 [details]
the patch
Comment 17 EWS Watchlist 2017-11-19 09:17:37 PST
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.
Comment 18 Filip Pizlo 2017-11-25 18:37:03 PST
Created attachment 327580 [details]
the patch
Comment 19 EWS Watchlist 2017-11-25 18:39:34 PST
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.
Comment 20 Saam Barati 2017-11-28 16:15:33 PST
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.
Comment 21 Filip Pizlo 2017-11-28 17:25:53 PST
(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.
Comment 22 Filip Pizlo 2017-11-29 20:35:49 PST
(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.
Comment 23 Filip Pizlo 2017-11-29 20:40:16 PST
Landed in https://trac.webkit.org/changeset/225314/webkit
Comment 24 Radar WebKit Bug Importer 2017-11-29 20:41:29 PST
<rdar://problem/35765225>
Comment 25 Yusuke Suzuki 2017-11-30 02:31:48 PST
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.
Comment 26 Alexey Proskuryakov 2017-12-04 21:57:58 PST
This change added a lot of leaks on the bots (from ~400 Kb to over 100 Mb).
Comment 27 Filip Pizlo 2017-12-04 22:22:25 PST
(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.
Comment 28 Filip Pizlo 2017-12-05 08:55:54 PST
(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
Comment 29 Keith Miller 2018-08-28 18:00:05 PDT
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! :(