Bug 179288 - GC should support isoheaps
Summary: GC should support isoheaps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 179876
  Show dependency treegraph
 
Reported: 2017-11-04 14:41 PDT by Filip Pizlo
Modified: 2018-08-28 18:00 PDT (History)
13 users (show)

See Also:


Attachments
it's a start (22.80 KB, patch)
2017-11-04 14:42 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (37.64 KB, patch)
2017-11-04 16:33 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (63.09 KB, patch)
2017-11-05 13:07 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
close to done (68.80 KB, patch)
2017-11-06 14:56 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
almost done (141.02 KB, patch)
2017-11-07 11:34 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (157.82 KB, patch)
2017-11-07 13:37 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
jsc is compiling maybe (177.78 KB, patch)
2017-11-07 14:21 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more things compile (180.36 KB, patch)
2017-11-07 14:38 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it ran 3d-cube so it's probably solid (203.24 KB, patch)
2017-11-17 11:59 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (203.89 KB, patch)
2017-11-19 08:10 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (204.17 KB, patch)
2017-11-19 08:12 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (210.42 KB, patch)
2017-11-19 09:15 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (212.10 KB, patch)
2017-11-25 18:37 PST, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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! :(