Summary: | GC should support isoheaps | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2017-11-04 14:41:42 PDT
Created attachment 326045 [details]
it's a start
Created attachment 326052 [details]
a bit more
Created attachment 326076 [details]
more
Created attachment 326160 [details]
close to done
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.
Created attachment 326254 [details]
more
it's starting to compile!
Created attachment 326260 [details]
jsc is compiling maybe
Created attachment 326264 [details]
more things compile
Created attachment 327208 [details]
it ran 3d-cube so it's probably solid
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.
(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. Created attachment 327342 [details]
the patch
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.
Created attachment 327343 [details]
the patch
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.
Created attachment 327344 [details]
the patch
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.
Created attachment 327580 [details]
the patch
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 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. (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. (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 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. This change added a lot of leaks on the bots (from ~400 Kb to over 100 Mb). (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. (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 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! :( |