Bug 233782 - [libpas] Update to 96f9f4c28dc119695311c7c6bd81ed1f3f4e260c (allow more specialization of partial versus exclusive allocation)
Summary: [libpas] Update to 96f9f4c28dc119695311c7c6bd81ed1f3f4e260c (allow more speci...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-02 13:16 PST by Filip Pizlo
Modified: 2021-12-03 09:10 PST (History)
3 users (show)

See Also:


Attachments
the patch (314.04 KB, patch)
2021-12-02 13:29 PST, Filip Pizlo
ysuzuki: 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 2021-12-02 13:16:03 PST
...
Comment 1 Filip Pizlo 2021-12-02 13:29:45 PST
Created attachment 445769 [details]
the patch
Comment 2 Yusuke Suzuki 2021-12-02 20:35:23 PST
Comment on attachment 445769 [details]
the patch

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

r=me
Can you add new files to bmalloc/CMakeLists.txt too?

> Source/bmalloc/ChangeLog:14
> +        of the same megapages, so a megapage lookup could not tell you if you were explicit or shared.

explicit => exclusive?

> Source/bmalloc/ChangeLog:17
> +        explicit and shared, and spreads it across all page management logic. So:

Ditto.

> Source/bmalloc/ChangeLog:20
> +        -> Page allocation knows the page's role, so it can use different megapages for explicit or shared. This
> +           enables explicit and shared to have different deallocation fast paths.

Ditto.

> Source/bmalloc/ChangeLog:22
> +        -> Page_kind is now different depending on whether the page is explicit or shared.

Ditto.

> Source/bmalloc/libpas/src/libpas/pas_local_allocator.c:62
> +        PAS_ASSERT(pas_is_aligned(allocator->object_size, pas_local_allocator_alignment(allocator)));

Should we move this to the subsequent `pas_segregated_size_directory_is_bitfit(directory)`'s else branch?
Comment 3 Filip Pizlo 2021-12-03 09:04:27 PST
(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 445769 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445769&action=review
> 
> r=me
> Can you add new files to bmalloc/CMakeLists.txt too?

Fixed!

> 
> > Source/bmalloc/ChangeLog:14
> > +        of the same megapages, so a megapage lookup could not tell you if you were explicit or shared.
> 
> explicit => exclusive?

Fixed all these

> 
> > Source/bmalloc/ChangeLog:17
> > +        explicit and shared, and spreads it across all page management logic. So:
> 
> Ditto.
> 
> > Source/bmalloc/ChangeLog:20
> > +        -> Page allocation knows the page's role, so it can use different megapages for explicit or shared. This
> > +           enables explicit and shared to have different deallocation fast paths.
> 
> Ditto.
> 
> > Source/bmalloc/ChangeLog:22
> > +        -> Page_kind is now different depending on whether the page is explicit or shared.
> 
> Ditto.
> 
> > Source/bmalloc/libpas/src/libpas/pas_local_allocator.c:62
> > +        PAS_ASSERT(pas_is_aligned(allocator->object_size, pas_local_allocator_alignment(allocator)));
> 
> Should we move this to the subsequent
> `pas_segregated_size_directory_is_bitfit(directory)`'s else branch?

I considered it, but this assertion is about alignment and object_size, which we just set right above the if+ASSERT.  So, the value of having it closer to where the logic is for setting object_size/alignment outweighs sharing the same if/else block, in my opinion.
Comment 4 Filip Pizlo 2021-12-03 09:09:25 PST
Landed in https://trac.webkit.org/changeset/286493/webkit
Comment 5 Radar WebKit Bug Importer 2021-12-03 09:10:23 PST
<rdar://problem/86020703>