Bug 131170 - bmalloc
Summary: bmalloc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-03 10:51 PDT by Geoffrey Garen
Modified: 2014-04-08 12:27 PDT (History)
10 users (show)

See Also:


Attachments
archive (196.77 KB, application/octet-stream)
2014-04-03 10:52 PDT, Geoffrey Garen
kling: review+
Details
Patch (5.82 KB, patch)
2014-04-07 17:45 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2014-04-03 10:51:52 PDT
This is bmalloc. This is just the library; it isn't integrated into WebKit or build-webkit yet.
Comment 1 Geoffrey Garen 2014-04-03 10:52:40 PDT
Created attachment 228521 [details]
archive

Uploading as a .zip because these are all new files.
Comment 2 WebKit Commit Bot 2014-04-03 10:55:57 PDT
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.
Comment 3 Andreas Kling 2014-04-03 15:50:24 PDT
Calls to mmap() should be tagged with VM_TAG_FOR_TCMALLOC_MEMORY (and someone should re-name that tag :|)
Comment 4 Sam Weinig 2014-04-03 16:56:42 PDT
(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.
Comment 5 Gavin Barraclough 2014-04-03 17:11:24 PDT
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;
Comment 6 Gavin Barraclough 2014-04-03 17:12:10 PDT
Oh, also, this project contains 25 tab characters. Please to be removing them and replacing with one hundred spaces.
Comment 7 Gavin Barraclough 2014-04-03 17:16:20 PDT
Surely INLINE & NO_INLINE could live in peace and harmony together sharing a header file? – I don't think they need one each.
Comment 8 Gavin Barraclough 2014-04-03 17:34:59 PDT
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.
Comment 9 Geoffrey Garen 2014-04-03 18:14:12 PDT
Can I distribute the spaces how I like?
Comment 10 Geoffrey Garen 2014-04-03 18:31:27 PDT
(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.
Comment 11 Geoffrey Garen 2014-04-03 18:37:46 PDT
(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.
Comment 12 Geoffrey Garen 2014-04-03 18:39:55 PDT
(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.
Comment 13 Geoffrey Garen 2014-04-03 18:41:26 PDT
(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.
Comment 14 Geoffrey Garen 2014-04-03 18:43:51 PDT
Also: Added 2-clause BSD licenses to all files.

Also: Renamed vmAlignment to superChunkSize.
Comment 15 Gavin Barraclough 2014-04-03 18:48:25 PDT
vmDeallocatePhysicalPages/vmAllocatePhysicalPages

Should these also ASSERT that p is page aligned?
Comment 16 Geoffrey Garen 2014-04-03 18:50:40 PDT
(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.
Comment 17 Geoffrey Garen 2014-04-03 18:56:36 PDT
(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.
Comment 18 Gavin Barraclough 2014-04-04 00:30:56 PDT
Vector<T>::pop() never shrinks the capacity?
Comment 19 Geoffrey Garen 2014-04-04 09:38:36 PDT
(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
Comment 20 Geoffrey Garen 2014-04-07 16:55:08 PDT
Committed revision 166893.
Comment 21 Geoffrey Garen 2014-04-07 17:45:50 PDT
Reopening to attach new patch.
Comment 22 Geoffrey Garen 2014-04-07 17:45:53 PDT
Created attachment 228785 [details]
Patch
Comment 23 Geoffrey Garen 2014-04-07 17:47:27 PDT
Comment on attachment 228785 [details]
Patch

Oops! Wrong bug for this patch.
Comment 24 Csaba Osztrogonác 2014-04-08 03:26:14 PDT
You missed to add copyright and license to AsyncTask.cpp.
Comment 25 Geoffrey Garen 2014-04-08 12:27:05 PDT
(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.