ARM: Do not stack allocate big buffers in Assembler.

ARM: Do not stack allocate big buffers in Assembler.

Currently Assembler stack allocates 120KB for pending reloc infos.
This can lead to stack-overflow in C++ since the stack guard limit
is only 40K smaller than the stack size.

BUG=405338
LOG=Y

[ulan, 2014-09-18 12:52:41]

ulan@chromium.org changed reviewers:
+ rmcilroy@chromium.org, rodolph.perfetta@arm.com

[ulan, 2014-09-18 12:52:42]

Hi Ross, Rodolph.

Could you please take a look? This fixes chrome crashes. Another solution would
be to reduce FLAG_stack_size by 120KB for ARM.

I will check benchmarks before landing.

[rmcilroy, 2014-09-18 13:02:03]

On 2014/09/18 12:52:42, ulan wrote:
> Hi Ross, Rodolph.
> 
> Could you please take a look? This fixes chrome crashes. Another solution
would
> be to reduce FLAG_stack_size by 120KB for ARM.
> 
> I will check benchmarks before landing.

This seems fine to me, but I would just always allocate
kMaxNumPending32/64RelocInfo if you are wanting to be cautious (for merge into
m38).  If malloc is smart enough to keep the allocation in a size-binned
free-list then this would be both faster (since it would always just re-use the
same memory) and potentially use less memory since it wouldn't cause
fragmentation due to different size being requested.  WDYT?

[ulan, 2014-09-18 13:36:27]

On 2014/09/18 13:02:03, rmcilroy wrote:
> On 2014/09/18 12:52:42, ulan wrote:
> > Hi Ross, Rodolph.
> > 
> > Could you please take a look? This fixes chrome crashes. Another solution
> would
> > be to reduce FLAG_stack_size by 120KB for ARM.
> > 
> > I will check benchmarks before landing.
> 
> This seems fine to me, but I would just always allocate
> kMaxNumPending32/64RelocInfo if you are wanting to be cautious (for merge into
> m38).  If malloc is smart enough to keep the allocation in a size-binned
> free-list then this would be both faster (since it would always just re-use
the
> same memory) and potentially use less memory since it wouldn't cause
> fragmentation due to different size being requested.  WDYT?

Thanks! I'll think about this, not sure how to test if malloc is smart.

I ran Octane and collected how many different buffer_sizes are requested:
cnt: 59341, buffer size: 256
cnt: 21086, buffer size: 44
cnt: 13577, buffer size: 40
cnt: 10101, buffer size: 4096
cnt: 2436, buffer size: 36
cnt: 364, buffer size: 2164
cnt: 224, buffer size: 2116
cnt: 204, buffer size: 1024
cnt: 78, buffer size: 884
cnt: 72, buffer size: 1972
cnt: 66, buffer size: 2100
cnt: 46, buffer size: 2132
cnt: 42, buffer size: 2148
cnt: 32, buffer size: 876
cnt: 30, buffer size: 868
cnt: 24, buffer size: 1924
cnt: 18, buffer size: 860
cnt: 16, buffer size: 852
cnt: 16, buffer size: 776
cnt: 16, buffer size: 2196
cnt: 16, buffer size: 16384
cnt: 8, buffer size: 772
cnt: 8, buffer size: 2180
cnt: 8, buffer size: 1892
cnt: 6, buffer size: 1876

[Sven Panne, 2014-09-18 13:48:22]

On 2014/09/18 13:02:03, rmcilroy wrote:
> This seems fine to me, but I would just always allocate
> kMaxNumPending32/64RelocInfo if you are wanting to be cautious (for merge into
> m38).  If malloc is smart enough to keep the allocation in a size-binned
> free-list then this would be both faster (since it would always just re-use
the
> same memory) and potentially use less memory since it wouldn't cause
> fragmentation due to different size being requested.  WDYT?

KISS: Trying to outsmart some unknown new[] implementation is a bad idea, and
failed in the end in every project I've seen so far.

[rmcilroy, 2014-09-18 14:02:25]

On 2014/09/18 13:48:22, Sven Panne wrote:
> On 2014/09/18 13:02:03, rmcilroy wrote:
> > This seems fine to me, but I would just always allocate
> > kMaxNumPending32/64RelocInfo if you are wanting to be cautious (for merge
into
> > m38).  If malloc is smart enough to keep the allocation in a size-binned
> > free-list then this would be both faster (since it would always just re-use
> the
> > same memory) and potentially use less memory since it wouldn't cause
> > fragmentation due to different size being requested.  WDYT?
> 
> KISS: Trying to outsmart some unknown new[] implementation is a bad idea, and
> failed in the end in every project I've seen so far.

I wasn't suggesting trying to out-smart malloc (I was not suggesting that we do
the size binning), I was just mentioning this since I don't necessarily think
it's worth having the extra complexity of allocating different sized buffers
given that we are only likely to have one (or a small number) of masm objects
about at any one time.  It doesn't really add much complexity though so I'm fine
with this either way.

lgtm.

[Rodolph Perfetta, 2014-09-18 14:03:24]

rodolph.perfetta@gmail.com changed reviewers:
+ rodolph.perfetta@gmail.com

[Rodolph Perfetta, 2014-09-18 14:03:25]

Relocinfo contains more info than is necessary for literal pools, so using a
dedicated structure (like in arm64) would also save space. That being said it is
only worth doing if the inline pool remains supproted.

https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:477: max_num_32_bit_reloc_info_ =
This may not work. kMaxNumPending[32|64]RelocInfo were define as
LiteralPoolRange / InstructionSize so that you never need to check if you are
going to overflow the array of RelocInfo since the pool will be emitted before
that happens. There is no such guarantee now. And if buffer_size_ is bigger than
4k then we are wasting space.

[Rodolph Perfetta, 2014-09-19 08:38:59]

On 2014/09/18 14:03:25, Rodolph Perfetta wrote:
> Relocinfo contains more info than is necessary for literal pools, so using a
> dedicated structure (like in arm64) would also save space. That being said it
is
> only worth doing if the inline pool remains supproted.
> 
> https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (right):
> 
>
https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc#new...
> src/arm/assembler-arm.cc:477: max_num_32_bit_reloc_info_ =
> This may not work. kMaxNumPending[32|64]RelocInfo were define as
> LiteralPoolRange / InstructionSize so that you never need to check if you are
> going to overflow the array of RelocInfo since the pool will be emitted before
> that happens. There is no such guarantee now. And if buffer_size_ is bigger
than
> 4k then we are wasting space.

OK scratch that, I missed the Min ...

Sorry.

[ulan, 2014-09-19 09:39:07]

On 2014/09/19 08:38:59, Rodolph Perfetta wrote:
> On 2014/09/18 14:03:25, Rodolph Perfetta wrote:
> > Relocinfo contains more info than is necessary for literal pools, so using a
> > dedicated structure (like in arm64) would also save space. That being said
it
> is
> > only worth doing if the inline pool remains supproted.
> > 
> > https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc
> > File src/arm/assembler-arm.cc (right):
> > 
> >
>
https://codereview.chromium.org/555943003/diff/1/src/arm/assembler-arm.cc#new...
> > src/arm/assembler-arm.cc:477: max_num_32_bit_reloc_info_ =
> > This may not work. kMaxNumPending[32|64]RelocInfo were define as
> > LiteralPoolRange / InstructionSize so that you never need to check if you
are
> > going to overflow the array of RelocInfo since the pool will be emitted
before
> > that happens. There is no such guarantee now. And if buffer_size_ is bigger
> than
> > 4k then we are wasting space.
> 
> OK scratch that, I missed the Min ...
> 
> Sorry

After offline discussions, I am inclined to go with adjusting the stack limit.
That would be safer to merge back to 3.28 and wouldn't introduce new performance
regressions: https://codereview.chromium.org/583163002/

If you agree, I'll abandon this CL.