Bug 200103 – [bmalloc] Add IsoHeap test to ensure that IsoHeap pages are not allocating too large VA

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[bmalloc] Add IsoHeap test to ensure that IsoHeap pages are not allocating too large VA

[Yusuke Suzuki]

<rdar://problem/53521391>

[Yusuke Suzuki]

Created attachment 374841 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:38:  mach_vm_allocate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:39:  mach_vm_deallocate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:40:  mach_vm_map is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:42:  mach_vm_protect is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:43:  mach_vm_region is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:45:  mach_vm_region_recurse is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:46:  mach_vm_purgable_control is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 19 files


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

[Mark Lam]

Comment on attachment 374841 [details]
Patch

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

r=me

> Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj:-46
> -		0C2DA1481F3BEB4900DBC317 /* MachVMSPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 0C2DA12C1F3BEB4900DBC317 /* MachVMSPI.h */; };

I don't see the old MachVMSPI.h being deleted.  Are you missing a "svn rm"?

[Yusuke Suzuki]

Comment on attachment 374841 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp:2474
> +    for (unsigned i = 0; i < 100; ++i)

I'll add a comment that this is exhausting IsoHeap shared tier.

> Tools/TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp:2490
> +    EXPECT_LT((static_cast<int64_t>(after[VM_MEMORY_JAVASCRIPT_JIT_REGISTER_FILE].reserved) - static_cast<int64_t>(before[VM_MEMORY_JAVASCRIPT_JIT_REGISTER_FILE].reserved)) * static_cast<int64_t>(WTF::vmPageSize()), 100 * static_cast<int64_t>(WTF::vmPageSize()));

I'll add a comment like this,

// Previously, we have an issue that every thread leaks 1~ page. Underlying IsoHeap implementation can add some IsoPages during this test and it would change the value of this, but we can say that such an implementation change would not add 100 pages, and if we have a leak in IsoTLS, it is larger than 1000 pages and this test correctly catch the issue.

> Tools/TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp:2518
> +    EXPECT_LT((static_cast<int64_t>(after[VM_MEMORY_JAVASCRIPT_JIT_REGISTER_FILE].reserved) - static_cast<int64_t>(before[VM_MEMORY_JAVASCRIPT_JIT_REGISTER_FILE].reserved)) * static_cast<int64_t>(WTF::vmPageSize()), (50 << 20));

I'll add a comment like this,

// We picked 50MB here to make this test work even if the underlying implementation of IsoHeap is changed, like, adding more # of IsoHeap shared tier instances.
// Currently, this number says 31MB in macOS, so 50MB threshold can catch the issue if the catastrophic thing happens. For example, if we revert https://bugs.webkit.org/show_bug.cgi?id=200024,
// we will see 2GB VA allocations here, which can be caught by this test.

[Yusuke Suzuki]

Created attachment 374845 [details]
Patch

Patch for landing

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:38:  mach_vm_allocate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:39:  mach_vm_deallocate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:40:  mach_vm_map is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:42:  mach_vm_protect is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:43:  mach_vm_region is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:45:  mach_vm_region_recurse is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/cocoa/MachVMSPI.h:46:  mach_vm_purgable_control is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 19 files


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

[WebKit Commit Bot]

Comment on attachment 374845 [details]
Patch

Rejecting attachment 374845 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 374845, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12806511

[Yusuke Suzuki]

Committed r247815: <https://trac.webkit.org/changeset/247815>