Bug 131170 – bmalloc

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

This is bmalloc. This is just the library; it isn't integrated into WebKit or build-webkit yet.

[Geoffrey Garen]

Created attachment 228521 [details]
archive

Uploading as a .zip because these are all new files.

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Andreas Kling]

Calls to mmap() should be tagged with VM_TAG_FOR_TCMALLOC_MEMORY (and someone should re-name that tag :|)

[Sam Weinig]

(In reply to comment #3)
> Calls to mmap() should be tagged with VM_TAG_FOR_TCMALLOC_MEMORY (and someone should re-name that tag :|)

Or better yet, we should get a new tag for this.

[Gavin Barraclough]

Mutex::isLocked()

It looks like this function may return the wrong results – specifically if the mutux is unlocked & two threads try to read the value at the same time, then one may return true.

Thread A:
    if (m_flag.test_and_set(std::memory_order_acquire))
        return true;

Thread B:
    if (m_flag.test_and_set(std::memory_order_acquire))
        return true;

Thread A:
    m_flag.clear();
    return false;

[Gavin Barraclough]

Oh, also, this project contains 25 tab characters. Please to be removing them and replacing with one hundred spaces.

[Gavin Barraclough]

Surely INLINE & NO_INLINE could live in peace and harmony together sharing a header file? – I don't think they need one each.

[Gavin Barraclough]

Style nit, "Sizes.h" is a mix of heap specific constants & stdlib-extra-esque helper functions (e.g. roundUpToMultipleOf). Some of the helper functions are depended on by stuff in the stdlib directory (which seems like a soft form of layering violation).

I think it would be cleaner to split this header in two, keep the bmalloc specific constants under heap, & move the helper functions to a new header in the stdlib directory.

[Geoffrey Garen]

Can I distribute the spaces how I like?

[Geoffrey Garen]

(In reply to comment #3)
> Calls to mmap() should be tagged with VM_TAG_FOR_TCMALLOC_MEMORY (and someone should re-name that tag :|)

Fixed locally.

[Geoffrey Garen]

(In reply to comment #5)
> Mutex::isLocked()
> 
> It looks like this function may return the wrong results – specifically if the mutux is unlocked & two threads try to read the value at the same time, then one may return true.

Good point.

I used to use this function for ASSERTs, in which case it's OK that one thread won't ASSERT, because the other thread will ASSERT, and then things will get synchronized right quick :).

If I wanted to keep this function, maybe I should move the ASSERT into it, or rename it to "isLockedSloppy" or something like that.

However, now that we have the "pass the lock to prove you have it" idiom, I guess I should just remove this function to avoid future foot-gunning. 

Fixed locally.

[Geoffrey Garen]

(In reply to comment #6)
> Oh, also, this project contains 25 tab characters. Please to be removing them and replacing with one hundred spaces.

Fixed locally.

[Geoffrey Garen]

(In reply to comment #7)
> Surely INLINE & NO_INLINE could live in peace and harmony together sharing a header file? – I don't think they need one each.

Bokay.

[Geoffrey Garen]

Also: Added 2-clause BSD licenses to all files.

Also: Renamed vmAlignment to superChunkSize.

[Gavin Barraclough]

vmDeallocatePhysicalPages/vmAllocatePhysicalPages

Should these also ASSERT that p is page aligned?

[Geoffrey Garen]

(In reply to comment #8)
> I think it would be cleaner to split this header in two, keep the bmalloc specific constants under heap, & move the helper functions to a new header in the stdlib directory.

Fixed locally: std/Algorithm.h.

[Geoffrey Garen]

(In reply to comment #15)
> vmDeallocatePhysicalPages/vmAllocatePhysicalPages
> 
> Should these also ASSERT that p is page aligned?

Bokay. Fixed locally by adding a "vmValidate" function for pointers and sizes.

[Gavin Barraclough]

Vector<T>::pop() never shrinks the capacity?

[Geoffrey Garen]

(In reply to comment #18)
> Vector<T>::pop() never shrinks the capacity?

Nice. Fixed locally:

Peak Memory:
    <geometric mean>                   17,340kB         17,206kB         ^ 1.01x smaller

Memory at End:
    churn --parallel                   480kB            452kB            ^ 1.06x smaller
    list_allocate --parallel           1,480kB          1,392kB          ^ 1.06x smaller
    tree_allocate --parallel           1,976kB          1,748kB          ^ 1.13x smaller
    tree_churn --parallel              2,056kB          1,888kB          ^ 1.09x smaller
    fragment_iterate --parallel        1,068kB          1,012kB          ^ 1.06x smaller
    medium                             8,344kB          5,488kB          ^ 1.52x smaller
    medium --parallel                  8,608kB          6,548kB          ^ 1.31x smaller

[Geoffrey Garen]

Committed revision 166893.

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 228785 [details]
Patch

[Geoffrey Garen]

Comment on attachment 228785 [details]
Patch

Oops! Wrong bug for this patch.

[Csaba Osztrogonác]

You missed to add copyright and license to AsyncTask.cpp.

[Geoffrey Garen]

(In reply to comment #24)
> You missed to add copyright and license to AsyncTask.cpp.

Thanks. That file is vestigial, so I've removed it.