Bug 55204 - malloc in removeChildren shows up on profile of peacekeeper domDynamicCreationCreateElement
Summary: malloc in removeChildren shows up on profile of peacekeeper domDynamicCreatio...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 48719
  Show dependency tree / graph
 
Reported: 2011-02-24 22:04 PST by Eric Seidel (no email)
Modified: 2011-02-27 09:01 PST (History)
6 users (show)

See Also:

Attachments
Patch (2.20 KB, patch)
2011-02-24 22:06 PST, Eric Seidel (no email) 		no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2011-02-24 22:59 PST, Eric Seidel (no email) 		no flags Details | Formatted Diff | Diff
Patch for landing (2.91 KB, patch)
2011-02-25 00:10 PST, Eric Seidel (no email) 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-02-24 22:04:01 PST 
malloc in removeChildren shows up on profile of peacekeeper domDynamicCreationCreateElement
Comment 1 Eric Seidel (no email) 2011-02-24 22:06:27 PST 
Created attachment 83771 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-02-24 22:07:38 PST 
Here are my benchmark numbers:

Before:

avg 396.96666666666664
median 399
stdev 6.554811633872905
min 375
max 402

After:

avg 386.26666666666665
median 388
stdev 6.303085664092541
min 368
max 395


I'm not sure that I trust them though.  They're quite noisy.  That's 30 runs of (locally modified) peacekeeper domDynamicCreationCreateElement.
Comment 3 Eric Seidel (no email) 2011-02-24 22:59:10 PST 
Created attachment 83774 [details]
Patch
Comment 4 Maciej Stachowiak 2011-02-24 23:34:14 PST 
Comment on attachment 83774 [details]
Patch

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

r=me

> Source/WebCore/dom/ContainerNode.cpp:359
> +    // FIXME: We should tune the inlineCapacity.  Having this function call malloc
> +    // as a result of needing to expand capacity is expensive an shows up on benchmarks.

This FIXME is probably not super helpful. Just do your best to tune it now. Someone else can always tune more later.

> Source/WebCore/dom/ContainerNode.cpp:521
> +    // FIXME: We should tune the inlineCapacity.  Having this function call malloc
> +    // as a result of needing to expand capacity is expensive an shows up on benchmarks.

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:524
> +    // childNodeCount() results in a full walk of the nodes, but should be faster
> +    // than having to malloc twice if we have lots of kids.

Dup of a previous comment - maybe this version can be shorter?
Comment 5 Eric Seidel (no email) 2011-02-24 23:35:09 PST 
Comment on attachment 83774 [details]
Patch

Will fix.
Comment 6 Eric Seidel (no email) 2011-02-25 00:10:27 PST 
Created attachment 83777 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2011-02-26 05:59:52 PST 
Comment on attachment 83777 [details]
Patch for landing

Clearing flags on attachment: 83777

Committed r79781: <http://trac.webkit.org/changeset/79781>
Comment 8 WebKit Commit Bot 2011-02-26 05:59:57 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2011-02-27 09:01:27 PST 
http://trac.webkit.org/changeset/79781 might have broken GTK Linux 64-bit Debug