Bug 127067 - Remove ENABLE_GLOBAL_FASTMALLOC_NEW
Summary: Remove ENABLE_GLOBAL_FASTMALLOC_NEW
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: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks: 153812
  Show dependency treegraph
 
Reported: 2014-01-15 14:57 PST by Anders Carlsson
Modified: 2016-02-02 19:02 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.68 KB, patch)
2014-01-15 15:01 PST, Anders Carlsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2014-01-15 14:57:05 PST
Remove ENABLE_GLOBAL_FASTMALLOC_NEW
Comment 1 Anders Carlsson 2014-01-15 15:01:52 PST
Created attachment 221309 [details]
Patch
Comment 2 Geoffrey Garen 2014-01-15 15:07:04 PST
Comment on attachment 221309 [details]
Patch

r=me

Before landing, I think we need to test performance and make sure we're not missing any significant WTF_MAKE_FAST_ALLOCATED declarations.

Would also be nice just to set a breakpoint / add a shim method and print out all allocations in WebKit that miss FastMalloc, and drive to 0.
Comment 3 Geoffrey Garen 2014-01-15 15:07:39 PST
> Would also be nice just to set a breakpoint / add a shim method and print out all allocations in WebKit that miss FastMalloc, and drive to 0.

To clarify, I don't think we need to do this task before landing -- I just think it would be fun!
Comment 4 Anders Carlsson 2014-01-15 15:22:07 PST
I'm going to hold off on landing this until some PLT regressions are tracked down.
Comment 5 Stephanie Lewis 2014-01-24 00:17:43 PST
Haven't looked at memory numbers but the PLT shows this to be a significant improvement. :)
Comment 6 Geoffrey Garen 2014-01-24 10:50:15 PST
> Haven't looked at memory numbers but the PLT shows this to be a significant improvement. :)

Wat? What are the exact numbers?

I can't imagine why this might be a speedup.
Comment 7 Stephanie Lewis 2014-01-28 15:50:13 PST
Ran again with production roots and the patch is still slightly faster, but nothing to write home about.

A quick check with membuster shows that in the user scenario ~15 MB is now allocated with System Malloc where it used to be allocated with FastMalloc.  Given that I'll want to check how memory returning and fragmentation is affected.
Comment 8 Anders Carlsson 2014-01-28 15:54:55 PST
(In reply to comment #7)
> Ran again with production roots and the patch is still slightly faster, but nothing to write home about.

My guess is that it's just variance.

> A quick check with membuster shows that in the user scenario ~15 MB is now allocated with System Malloc where it used to be allocated with FastMalloc.  Given that I'll want to check how memory returning and fragmentation is affected.

This is interesting. Is there any way to compare the heaps to see what data we're now allocating with System Malloc (and fix the code so it uses fastMalloc instead)?
Comment 9 Stephanie Lewis 2014-01-28 16:07:16 PST
We could run membuster with malloc history enabled and write a script to compare the two and dump the differences.
Comment 10 Anders Carlsson 2014-02-17 16:52:11 PST
Turns out it's really easy to see where we call operator new() vs fastMalloc in Instruments. With the changes I landed in http://trac.webkit.org/projects/webkit/changeset/163757 we now allocated pretty much everything with fastMalloc (tested loading the WK1 spec). I'm going to land this.
Comment 11 WebKit Commit Bot 2014-02-17 17:27:06 PST
Comment on attachment 221309 [details]
Patch

Clearing flags on attachment: 221309

Committed r164261: <http://trac.webkit.org/changeset/164261>
Comment 12 WebKit Commit Bot 2014-02-17 17:27:09 PST
All reviewed patches have been landed.  Closing bug.