[Webkit-unassigned] [Bug 94712] High memory usage spike in AssemblerBuffer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94712





--- Comment #30 from Benjamin Poulain <benjamin at webkit.org>  2012-08-30 20:48:38 PST ---
(From update of attachment 161564)
View in context: https://bugs.webkit.org/attachment.cgi?id=161564&action=review

I am too tired to review this now. I think this looks very promising but it needs a lot more polishing.

Here are some quick comments.

> Source/JavaScriptCore/ChangeLog:13
> +        It is seen AssemberBuffer can use 20MB+ memory on Google's Octane
> +        page. When the buffer grows, it allocates a new memory block and makes a copy,
> +        which results in a huge memory spike. This patch makes AssemblerBuffer use
> +        a SegmentedBuffer instead, and this can reduce memory spikes and also avoid
> +        copying existing data when buffer grows.

This is a performance patch, you should have a performance analysis paragraph in your changelog.
1) You need to determine if it causes changes in the common benchmarks.
2) You need to gather numbers on how much memory improvement the patch makes. For example measuring the memory before-after patch with Sunspider and Octane.

Ideally, you should also have an analysis for the SegmentSize and InlineCapacity. It can be as simple as: "Running all the benchmarks, the average is XX segments and max at YY segments".

I haven't looked at the x86 assembler, and it might be obvious why this patch does not generalize to other assemblers.
Unless it is obviously wrong, I think you should do the implementation and measure the performance on x86_64. So far, I have only read very weak arguments and no numbers.

Remember this patch is touching important code, you want to make sure this is a net improvement.

> Source/JavaScriptCore/assembler/AssemblerBufferSegmented.h:38
> +        static const int inlineCapacity = 2048;

This does not do what you think it does. You are defining the Segment size. In:
    SegmentedVector<char, inlineCapacity> m_storage;
The first argument is the SegmentSize, the second is the inline capacity.

The inline capacity of SegmentedVector is for the vector listing the segments. 2048 would be huge.

For the segment size, chances are you want to match the page size of your allocator to avoid wasting memory. 
For fastMalloc, I would expose kPageSize and make sure sizeof(Segment) == kPageSize.

In this case, 2048 for the segment size seems not optimal for any allocator because the vector is gonna need the size for "size_t m_size;" in addition to the vector.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.