Bug 172736 – bmalloc: Some refactoring to prepare for optimization

• Component: New Bugs
• Status: NEW

[Geoffrey Garen]

bmalloc: Some refactoring to prepare for optimization

[Geoffrey Garen]

Created attachment 311546 [details]
Patch

[Michael Saboff]

Comment on attachment 311546 [details]
Patch

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

r=me

> Source/bmalloc/bmalloc/VMHeap.cpp:-79
> -    vmRevokePermissions(begin.address(), vmPageSize);
> -    vmRevokePermissions(end.address() - vmPageSize, vmPageSize);

Why do we no longer need guard pages around small chunks?

[Saam Barati]

Comment on attachment 311546 [details]
Patch

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

r=me too

> Source/bmalloc/bmalloc/VMHeap.h:49
> +    LargeRange tryAllocateLargeChunk(size_t alignment, size_t);

why remove the lock argument? Is it not needed to hold the lock when calling this?

[Geoffrey Garen]

> > Source/bmalloc/bmalloc/VMHeap.h:49
> > +    LargeRange tryAllocateLargeChunk(size_t alignment, size_t);
> 
> why remove the lock argument? Is it not needed to hold the lock when calling
> this?

I've started removing the lock arguments from internal data structures. So, public Heap APIs still indicate the lock requirement, but we don't pass that through to heap-internal classes.

One reason is that it's the parent object that uses a data structure in one way or another, so it's not really practical for the child data structures to know their locking requirements. 

Another reason is that there's a specific practical problem with the difference between std::lock_guard and std::unique_lock. Whenever you make a change to use one or the other, suddenly you have to edit like 50 different signatures.

[Geoffrey Garen]

> > Source/bmalloc/bmalloc/VMHeap.cpp:-79
> > -    vmRevokePermissions(begin.address(), vmPageSize);
> > -    vmRevokePermissions(end.address() - vmPageSize, vmPageSize);
> 
> Why do we no longer need guard pages around small chunks?

Doh!

That change is required for the optimization, but it's probably not appropriate for a refactoring patch. Unfortunately, it's really hard to tease apart. I'll have to think about this...

[Geoffrey Garen]

Created attachment 311572 [details]
Patch

[Geoffrey Garen]

Added back guard pages. Removed the change to how we initialize m_objectTypes for large objects because it causes free(malloc(HUGE)) to consume huge metadata memory.

[Geoffrey Garen]

Comment on attachment 311572 [details]
Patch

Seems to be crashing on EWS.