[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 #31 from Yong Li <yoli at rim.com>  2012-08-31 07:36:34 PST ---
(In reply to comment #30)
> (From update of attachment 161564 [details])
> 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.

The purpose of this patch is not to boost performance, but to reduce memory spike. I've noticed the buffer size can hit ~24MB. When it grows, it can run out-of-memory on a mobile device. If I was working on any x86 port, I would try using PageReservation to reserve a huge chunk of virtual memory (as RegisterFile does), and probably I would write another vector class that uses OSAllocator rather than malloc, so that it would keep the buffer consecutive, but avoid creating new buffers when it grows. But I don't think I should be responsible to implement this or test on x86 port as I'm not working on any x86 port.

That said, this is definitely a change that can affect performance. During my test, it gives 1-2% boost on sunspider when the segment size is 2048. 4096 doesn't give better result. I cannot see any impact on Octane. I can do more testing for sure.

I think we should always use a power of 2 as segment size because operations like (index / segmentSize) and (index % segmentSize) are usually very expensive, but if segmentSize is power of 2, compiler can optimize them with bit operations.

Speaking of best block size for memory allocation, even pageSize is probably not the best size for malloc, depending on the implementation, the malloc may need extra space for block header. Using OSAllocator to allocate pages should be the best solution. But that would need too much change. We could think about something like PageSegmentedVector and PageVector...

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