Bug 121045 - CopiedSpace::startedCopying should not call MarkedSpace::capacity
Summary: CopiedSpace::startedCopying should not call MarkedSpace::capacity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks: 121074
  Show dependency treegraph
 
Reported: 2013-09-09 13:51 PDT by Mark Hahnenberg
Modified: 2013-09-12 10:57 PDT (History)
0 users

See Also:


Attachments
Patch (1.51 KB, patch)
2013-09-09 13:52 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2013-09-09 17:13 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-09-09 13:51:27 PDT
MarkedSpace::capacity() iterates every block in MarkedSpace. Heap has m_totalBytesVisited, which is a good enough approximation of the size of MarkedSpace and it's O(1) so we should use it instead.
Comment 1 Mark Hahnenberg 2013-09-09 13:52:38 PDT
Created attachment 211079 [details]
Patch
Comment 2 Mark Hahnenberg 2013-09-09 17:13:21 PDT
Created attachment 211123 [details]
Patch
Comment 3 Mark Hahnenberg 2013-09-09 17:14:45 PDT
I was getting a small but persistent slow down on kraken with the first version, so I tried just keeping track as we add and remove MarkedBlocks from the MarkedSpace. This version didn't show any slowdown.
Comment 4 Geoffrey Garen 2013-09-09 17:17:35 PDT
Comment on attachment 211123 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/MarkedSpace.h:135
> +    size_t m_totalCapacity;

I think "capacity" is a slightly better name here. We don't have a concept of "partial capacity" in this class.
Comment 5 Mark Hahnenberg 2013-09-09 17:34:19 PDT
Committed r155406: <http://trac.webkit.org/changeset/155406>