Bug 184121 – bmalloc should compute its own estimate of its footprint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 336742 [details]
WIP

This is just a stupid implementation that walks a bunch of data structures each time. Using this to do some testing. We should probably not make it walk everything every time we ask for footprint...

[Saam Barati]

Created attachment 336747 [details]
WIP

This time without the deadlock

[Saam Barati]

Created attachment 336812 [details]
WIP

I think I have the accounting down to do it in place now.

[Saam Barati]

(In reply to Saam Barati from comment #3)
> Created attachment 336812 [details]
> WIP
> 
> I think I have the accounting down to do it in place now.

I had to make LargeRange remember how many total physical bytes it represents, even if that may not account for the physical bytes at the beginning of the range.

[Saam Barati]

Created attachment 336826 [details]
WIP

[Saam Barati]

Created attachment 336846 [details]
WIP

Close to done. Just need to figure out concurrency in one place w.r.t IsoHeaps.

[Saam Barati]

Created attachment 336909 [details]
WIP

Now with bmalloc API to commit/decommit.

[Saam Barati]

Created attachment 336916 [details]
patch

I still need to do some performance testing on iOS. The footprint measurement is accurate, but unlike phys_footprint, it doesn't account for compressed memory on iOS. This means I may need to tune some of our isUnderMemoryPressure heuristics to account for the change in measurement.

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/bmalloc.h:100:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:101:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 23 files


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

[Saam Barati]

Created attachment 336917 [details]
patch

Try to fix some of the builds

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/bmalloc.h:100:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:101:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 23 files


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

[EWS Watchlist]

Comment on attachment 336917 [details]
patch

Attachment 336917 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7156164

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 336922 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Saam Barati]

(In reply to Saam Barati from comment #8)
> Created attachment 336916 [details]
> patch
> 
> I still need to do some performance testing on iOS. The footprint
> measurement is accurate, but unlike phys_footprint, it doesn't account for
> compressed memory on iOS. This means I may need to tune some of our
> isUnderMemoryPressure heuristics to account for the change in measurement.

Perf seemed fine on JetStream. I'll let the bots handle testing many different devices.

[Saam Barati]

Created attachment 337008 [details]
patch

test EWS again.

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/bmalloc.h:100:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:101:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 23 files


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

[WebKit Commit Bot]

Comment on attachment 337008 [details]
patch

Clearing flags on attachment: 337008

Committed r230187: <https://trac.webkit.org/changeset/230187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39118176>

[Ryan Haddad]

This change seems to have caused flaky crashes on macOS Debug WK1 bots:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000108481175 bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long, unsigned long) + 133 (LargeRange.h:57)
1   com.apple.JavaScriptCore      	0x000000010847d115 bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long, unsigned long) + 53 (LargeRange.h:59)
2   com.apple.JavaScriptCore      	0x000000010848049a bmalloc::LargeRange::split(unsigned long) const + 1226 (LargeRange.h:138)
3   com.apple.JavaScriptCore      	0x000000010847fc97 bmalloc::Heap::splitAndAllocate(std::__1::lock_guard<bmalloc::StaticMutex>&, bmalloc::LargeRange&, unsigned long, unsigned long) + 407 (Heap.cpp:479)
4   com.apple.JavaScriptCore      	0x000000010848099a bmalloc::Heap::tryAllocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&, unsigned long, unsigned long) + 698 (Heap.cpp:541)
5   com.apple.JavaScriptCore      	0x0000000108480a3d bmalloc::Heap::allocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&, unsigned long, unsigned long) + 45 (Heap.cpp:546)
6   com.apple.JavaScriptCore      	0x000000010847535b bmalloc::Allocator::allocateLarge(unsigned long) + 107 (Allocator.cpp:172)
7   com.apple.JavaScriptCore      	0x000000010847566a bmalloc::Allocator::allocateSlowCase(unsigned long) + 362 (Allocator.cpp:199)
8   com.apple.JavaScriptCore      	0x00000001083e1e68 bmalloc::Allocator::allocate(unsigned long) + 72 (Allocator.h:91)
9   com.apple.JavaScriptCore      	0x00000001083e1db6 bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) + 166 (Cache.h:79)
10  com.apple.JavaScriptCore      	0x00000001083e157b bmalloc::api::malloc(unsigned long, bmalloc::HeapKind) + 27 (bmalloc.h:49)


Full crash log here:
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r230190%20(7008)/results.html

[Saam Barati]

(In reply to Ryan Haddad from comment #20)
> This change seems to have caused flaky crashes on macOS Debug WK1 bots:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x0000000108481175
> bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long,
> unsigned long) + 133 (LargeRange.h:57)
> 1   com.apple.JavaScriptCore      	0x000000010847d115
> bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long,
> unsigned long) + 53 (LargeRange.h:59)
> 2   com.apple.JavaScriptCore      	0x000000010848049a
> bmalloc::LargeRange::split(unsigned long) const + 1226 (LargeRange.h:138)
> 3   com.apple.JavaScriptCore      	0x000000010847fc97
> bmalloc::Heap::splitAndAllocate(std::__1::lock_guard<bmalloc::StaticMutex>&,
> bmalloc::LargeRange&, unsigned long, unsigned long) + 407 (Heap.cpp:479)
> 4   com.apple.JavaScriptCore      	0x000000010848099a
> bmalloc::Heap::tryAllocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&,
> unsigned long, unsigned long) + 698 (Heap.cpp:541)
> 5   com.apple.JavaScriptCore      	0x0000000108480a3d
> bmalloc::Heap::allocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&,
> unsigned long, unsigned long) + 45 (Heap.cpp:546)
> 6   com.apple.JavaScriptCore      	0x000000010847535b
> bmalloc::Allocator::allocateLarge(unsigned long) + 107 (Allocator.cpp:172)
> 7   com.apple.JavaScriptCore      	0x000000010847566a
> bmalloc::Allocator::allocateSlowCase(unsigned long) + 362 (Allocator.cpp:199)
> 8   com.apple.JavaScriptCore      	0x00000001083e1e68
> bmalloc::Allocator::allocate(unsigned long) + 72 (Allocator.h:91)
> 9   com.apple.JavaScriptCore      	0x00000001083e1db6
> bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) + 166 (Cache.h:79)
> 10  com.apple.JavaScriptCore      	0x00000001083e157b
> bmalloc::api::malloc(unsigned long, bmalloc::HeapKind) + 27 (bmalloc.h:49)
> 
> 
> Full crash log here:
> https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/
> r230190%20(7008)/results.html

Interesting. I'll look into this. These asserts I added should always hold, so the bug is probably elsewhere if they don't hold.

[Saam Barati]

(In reply to Saam Barati from comment #21)
> (In reply to Ryan Haddad from comment #20)
> > This change seems to have caused flaky crashes on macOS Debug WK1 bots:
> > 
> > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> > 0   com.apple.JavaScriptCore      	0x0000000108481175
> > bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long,
> > unsigned long) + 133 (LargeRange.h:57)
> > 1   com.apple.JavaScriptCore      	0x000000010847d115
> > bmalloc::LargeRange::LargeRange(void*, unsigned long, unsigned long,
> > unsigned long) + 53 (LargeRange.h:59)
> > 2   com.apple.JavaScriptCore      	0x000000010848049a
> > bmalloc::LargeRange::split(unsigned long) const + 1226 (LargeRange.h:138)
> > 3   com.apple.JavaScriptCore      	0x000000010847fc97
> > bmalloc::Heap::splitAndAllocate(std::__1::lock_guard<bmalloc::StaticMutex>&,
> > bmalloc::LargeRange&, unsigned long, unsigned long) + 407 (Heap.cpp:479)
> > 4   com.apple.JavaScriptCore      	0x000000010848099a
> > bmalloc::Heap::tryAllocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&,
> > unsigned long, unsigned long) + 698 (Heap.cpp:541)
> > 5   com.apple.JavaScriptCore      	0x0000000108480a3d
> > bmalloc::Heap::allocateLarge(std::__1::lock_guard<bmalloc::StaticMutex>&,
> > unsigned long, unsigned long) + 45 (Heap.cpp:546)
> > 6   com.apple.JavaScriptCore      	0x000000010847535b
> > bmalloc::Allocator::allocateLarge(unsigned long) + 107 (Allocator.cpp:172)
> > 7   com.apple.JavaScriptCore      	0x000000010847566a
> > bmalloc::Allocator::allocateSlowCase(unsigned long) + 362 (Allocator.cpp:199)
> > 8   com.apple.JavaScriptCore      	0x00000001083e1e68
> > bmalloc::Allocator::allocate(unsigned long) + 72 (Allocator.h:91)
> > 9   com.apple.JavaScriptCore      	0x00000001083e1db6
> > bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) + 166 (Cache.h:79)
> > 10  com.apple.JavaScriptCore      	0x00000001083e157b
> > bmalloc::api::malloc(unsigned long, bmalloc::HeapKind) + 27 (bmalloc.h:49)
> > 
> > 
> > Full crash log here:
> > https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/
> > r230190%20(7008)/results.html
> 
> Interesting. I'll look into this. These asserts I added should always hold,
> so the bug is probably elsewhere if they don't hold.

This might actually be a bug in my code with how I handle rounding.

[Saam Barati]

Fix coming in:
https://bugs.webkit.org/show_bug.cgi?id=184275

[Geoffrey Garen]

Comment on attachment 337008 [details]
patch

Seems wrong to me that bmalloc and Iso subspaces need to communicate about madvise. If Iso subspaces want isolated VM whose physical pages they manage, and whose virtual pages are never shared with other objects, they should just call mmap() themselves and not use bmalloc.