Bug 151817 - bmalloc: extra large allocations could be more efficient
Summary: bmalloc: extra large allocations could be more efficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks: 153923
  Show dependency treegraph
 
Reported: 2015-12-03 12:29 PST by Michael Saboff
Modified: 2016-02-09 18:50 PST (History)
1 user (show)

See Also:


Attachments
Patch (4.14 KB, patch)
2015-12-03 12:34 PST, Michael Saboff
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
Removed debugging include to fix build (3.99 KB, patch)
2015-12-03 13:24 PST, Michael Saboff
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (3.97 KB, patch)
2015-12-03 13:31 PST, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2015-12-03 12:29:56 PST
BMalloc aligns extra large allocations to start on super chunk size aligned addresses.  It does this by increasing the allocation size to be at least two super chunks and then deallocating the front and back memory to get the start address to be super chunk aligned.  Given that the super chunk size is 4MB, we always ask the OS for 8MB for allocations 2MB or more and return the unused.  It makes sense to reduce the super chunk size to 2MB.

Secondly, when realloc is called on an extra large allocated piece of memory, we always allocate new memory and memcpy from the old.  There is a good possibility that we can get the memory for the increase from adjacently allocated memory, thus saving having both region allocated at once and the memcpy.
Comment 1 Michael Saboff 2015-12-03 12:30:41 PST
<rdar://problem/23639897>
Comment 2 Michael Saboff 2015-12-03 12:34:14 PST
Created attachment 266545 [details]
Patch
Comment 3 Geoffrey Garen 2015-12-03 12:49:20 PST
Comment on attachment 266545 [details]
Patch

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

r=me

> Source/bmalloc/bmalloc/VMAllocate.h:36
> +#include <wtf/DataLog.h>

Revert.

> Source/bmalloc/bmalloc/VMAllocate.h:92
> +    if (vmOldSize >= vmNewSize)
> +        return true;

I think this should be an ASSERT instead of a check.

This function doesn't truly honor a shrink in size, since it doesn't shrink the allocation. So it should not pretend to succeed.
Comment 4 Michael Saboff 2015-12-03 13:24:16 PST
Created attachment 266552 [details]
Removed debugging include to fix build
Comment 5 Geoffrey Garen 2015-12-03 13:28:20 PST
Comment on attachment 266552 [details]
Removed debugging include to fix build

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

r=me

> Source/bmalloc/bmalloc/VMAllocate.h:91
> +    if (vmOldSize >= vmNewSize)
> +        return true;

Same comment about ASSERT here.
Comment 6 Michael Saboff 2015-12-03 13:31:21 PST
Created attachment 266553 [details]
Patch for landing

Didn't see the comments when I posted the last patch.  Changed the "if" into an assert as suggested.
Comment 7 WebKit Commit Bot 2015-12-03 13:45:07 PST
Comment on attachment 266553 [details]
Patch for landing

Clearing flags on attachment: 266553

Committed r193373: <http://trac.webkit.org/changeset/193373>
Comment 8 WebKit Commit Bot 2015-12-03 13:45:10 PST
All reviewed patches have been landed.  Closing bug.