Bug 231938 - Try to make all fastMalloc isoheaped
Summary: Try to make all fastMalloc isoheaped
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on: 233094 235347
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-18 21:26 PDT by Filip Pizlo
Modified: 2022-01-18 20:24 PST (History)
1 user (show)

See Also:


Attachments
it begins (237.95 KB, patch)
2021-10-18 21:27 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
so many WTF files compile (256.99 KB, patch)
2021-10-19 10:25 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
WTF compiles now (299.74 KB, patch)
2021-10-19 17:44 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
LLIntOffsetExtractor now compiles (330.01 KB, patch)
2021-10-19 20:42 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
made it up to unified source 55 or so (393.55 KB, patch)
2021-10-20 13:08 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
up to unified source 85 or so (437.00 KB, patch)
2021-10-20 22:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
up to unified source 112 or so (462.30 KB, patch)
2021-10-22 12:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
JavaScriptCore.framework builds (503.69 KB, patch)
2021-10-22 18:31 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
jsc shell builds (519.99 KB, patch)
2021-10-23 11:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
all of jsc builds (520.63 KB, patch)
2021-10-23 12:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it runs "hello world" successfully (524.17 KB, patch)
2021-10-23 15:21 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it passes all the tests and uses way too much memory and runs way too slow (594.38 KB, patch)
2021-10-26 18:46 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
status report after running typescript,acorn-wtb,Air,pdfjs,crypto-aes-SP from RAMification (4.94 MB, text/plain)
2021-10-26 18:48 PDT, Filip Pizlo
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2021-10-18 21:26:23 PDT
I'm holding my thumbs.
Comment 1 Filip Pizlo 2021-10-18 21:27:39 PDT
Created attachment 441688 [details]
it begins
Comment 2 Filip Pizlo 2021-10-19 10:25:23 PDT
Created attachment 441751 [details]
so many WTF files compile
Comment 3 Filip Pizlo 2021-10-19 17:44:02 PDT
Created attachment 441827 [details]
WTF compiles now
Comment 4 Filip Pizlo 2021-10-19 20:42:26 PDT
Created attachment 441842 [details]
LLIntOffsetExtractor now compiles
Comment 5 Filip Pizlo 2021-10-20 13:08:26 PDT
Created attachment 441925 [details]
made it up to unified source 55 or so
Comment 6 Filip Pizlo 2021-10-20 22:49:38 PDT
Created attachment 441989 [details]
up to unified source 85 or so
Comment 7 Filip Pizlo 2021-10-22 12:55:49 PDT
Created attachment 442187 [details]
up to unified source 112 or so
Comment 8 Filip Pizlo 2021-10-22 18:31:05 PDT
Created attachment 442239 [details]
JavaScriptCore.framework builds

Of course, jsc binary fails to build.  I'll get back to that...
Comment 9 Filip Pizlo 2021-10-23 11:22:53 PDT
Created attachment 442266 [details]
jsc shell builds

I haven't dared testing it
Comment 10 Filip Pizlo 2021-10-23 12:49:48 PDT
Created attachment 442276 [details]
all of jsc builds

Haven't tested it yet
Comment 11 Filip Pizlo 2021-10-23 14:56:53 PDT
And of course it crashes in a super subtle way.

I think that somewhere, this patch messes up the size of something.
Comment 12 Filip Pizlo 2021-10-23 15:20:22 PDT
Comment on attachment 442276 [details]
all of jsc builds

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

> Source/WTF/wtf/FastMalloc.h:276
> +template<typename T>
> +T* fastMallocArrayWithLength(size_t arrayLength)
> +{
> +    size_t size;
> +    if (__builtin_mul_overflow(arrayLength, sizeof(T), &size))
> +        CRASH();
> +    return fastMallocArrayWithByteSize<T>(arrayLength);
> +}

Compute the size, pass the arrayLength as the size anyway.

That's what I call quality!
Comment 13 Filip Pizlo 2021-10-23 15:21:54 PDT
Created attachment 442281 [details]
it runs "hello world" successfully

SHIP IT!!
Comment 14 Filip Pizlo 2021-10-23 16:57:43 PDT
The ghost of bad decisions past returns:

pas panic: /Volumes/Data/primary/OpenSource/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache_layout.c:73: pas_allocator_index pas_thread_local_cache_layout_add_node(pas_thread_local_cache_layout_node): assertion !did_overflow failed.

Seems like I'll have to:

- Make pas_allocator_index be 32-bit again. Or at least 24-bit.
- Do something to control blow-up of the size of thread_local_caches and size class lookup tables.

Not easy, but doable.  I guess first thing is to test pas_allocator_index being 32-bit again, and make sure that this makes tests pass.
Comment 15 Filip Pizlo 2021-10-24 11:35:22 PDT
(In reply to Filip Pizlo from comment #14)
> The ghost of bad decisions past returns:
> 
> pas panic:
> /Volumes/Data/primary/OpenSource/Source/bmalloc/libpas/src/libpas/
> pas_thread_local_cache_layout.c:73: pas_allocator_index
> pas_thread_local_cache_layout_add_node(pas_thread_local_cache_layout_node):
> assertion !did_overflow failed.
> 
> Seems like I'll have to:
> 
> - Make pas_allocator_index be 32-bit again. Or at least 24-bit.

Can't be 24-bit since we need to be able to set and get it atomically.

> - Do something to control blow-up of the size of thread_local_caches and
> size class lookup tables.

Easy: need to have decommit of unused parts of the thread local caches, and decommit of unused size class lookup tables.
Comment 16 Filip Pizlo 2021-10-24 18:27:30 PDT
LOL 17% RAMification overhead, is what it's looking like, right now.
Comment 17 Radar WebKit Bug Importer 2021-10-25 21:27:23 PDT
<rdar://problem/84646022>
Comment 18 Filip Pizlo 2021-10-26 18:46:43 PDT
Created attachment 442552 [details]
it passes all the tests and uses way too much memory and runs way too slow

it's a start
Comment 19 Filip Pizlo 2021-10-26 18:48:42 PDT
Created attachment 442553 [details]
status report after running typescript,acorn-wtb,Air,pdfjs,crypto-aes-SP from RAMification

This shows the pas_status_reporter dump after running typescript,acorn-wtb,Air,pdfjs,crypto-aes-SP.  In this test, using all these isoheaps increases memory footprint a lot - by maybe 20MB or so.  It also runs slower.  This libpas status report shows the 1097 heaps that we create and what they look like.

Lots of dumb stuff in there, like types that are really primitive, but we didn't detect them to be primitive.