Bug 233782 – [libpas] Update to 96f9f4c28dc119695311c7c6bd81ed1f3f4e260c (allow more specialization of partial versus exclusive allocation)

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 445769 [details]
the patch

[Yusuke Suzuki]

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?

[Filip Pizlo]

(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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/286493/webkit

[Radar WebKit Bug Importer]

<rdar://problem/86020703>