RFR: JDK-8205336: Modularize allocations in assembler

[Roman Kennke]

Am 21.06.2018 um 11:14 schrieb Erik Österlund:
> Hi Roman,
> 
> I'm looking at the x86 code. I noticed that We used to always call
> incr_allocated_bytes after calling eden_allocate. However, now you moved
> incr_allocated_bytes into eden_allocate, which makes sense, and I like
> that. But it is only incremented if inline contig alloc is supported,
> otherwise it calls the slowpath without incrementing allocation bytes.
> So that is seemingly changing the behaviour from before. Is that a bug
> in your code, a bug in the current code, or am I misunderstanding this
> code?

It did generate the code for incr_allocated_bytes(), but always
unconditionally jumped over it if inline contig alloc is not supported.
I wouldn't call it a bug, but it's bogus code.

 It looks to me that your code is fixing a bug in the case when not
> using TLAB and not using contig allocation. Today it seems to me that we
> will always call the slowpath that always increments allocated bytes,
> and then always account it one more time in the interpreter. There might
> be more subtle differences as well that I have not considered yet.
> Thoughts?

As far as I can tell, the incr_allocated_bytes() issue is the only
difference. And it's not really a bug, just a slight inefficiency in
that it generates some dead instructions.

> Otherwise, I think this looks reasonable.

Ok, thanks for reviewing. Can you verify that what I said above is
correct? I'll push it through Mach5 in the meantime.

Thanks,
Roman


> 
> Thanks,
> /Erik
> 
> Other than that, it looks reasonable.
> 
> On 2018-06-19 21:46, Roman Kennke wrote:
>> Am 19.06.2018 um 19:56 schrieb Andrew Haley:
>>> On 06/19/2018 06:14 PM, Roman Kennke wrote:
>>>> Can I please get reviews?
>>> AArch64 looks OK, but it makes no sense for Register thread to be an
>>> argument to eden_allocate: it's rthread.  Otherwise fine.
>>>
>>> Unless you've messed up copying some of the code, but it'd be hard
>>> to check all that by hand without half a day to spare...  :-(
>> Right. It was pre-existing (probably modeled after x86), and I wanted to
>> make it a 1:1 copy/refactoring job as much as possible, but yeah this is
>> pointless in aarch64. Let's remove it:
>>
>> Incremental:
>> http://cr.openjdk.java.net/~rkennke/JDK-8205336/webrev.01.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/JDK-8205336/webrev.01/
>>
>> Good now?
>>
>> Thanks for reviewing!
>> Roman
>>
>