[foreign-memaccess] RFR 8231402: layout API implementation is not constant enough

[Maurizio Cimadamore]

On 24/09/2019 19:59, Brian Goetz wrote:
> Some review comments on the class specs themselves, not just on this 
> particular change set.
>
> I am happy to see that you have raised the flag of value-based on 
> MemorySegment and friends.  But, as written, it's a bit of a lie, as 
> these types are interfaces, and only classes can be value-based.  What 
> you probably mean is: "all implementations of XXX must be 
> value-based."  The same is true when you say "this class is immutable 
> and thread-safe".
Thanks for pointing these out - I'll make sure these get addressed in a 
followup patch.
>
> I would add to this: "In the future, this will be a **sealed** type 
> ..."; you do this for MemoryLayout but not MemorySegment. But even in 
> MemoryLayout, I would strengthen to the wording used in j.l.constant:
I've looked at that - but then decided to only use parts of it, given 
there are no public API types that are truly exposed here. In any case, 
I'll strengthen what we have.
>
>  * <p>Non-platform classes should not implement {@linkplain 
> ConstantDesc} directly.
>  * Instead, they should extend {@link DynamicConstantDesc} (as {@link 
> EnumDesc}
>  * and {@link VarHandleDesc} do.)
>
>  * @apiNote In the future, if the Java language permits, {@linkplain 
> ConstantDesc}
>  * may become a {@code sealed} interface, which would prohibit 
> subclassing except by
>  * explicitly permitted types.  Clients can assume that the following
>  * set of subtypes is exhaustive: {@link String}, {@link Integer},
>  * {@link Long}, {@link Float}, {@link Double}, {@link ClassDesc},
>  * {@link MethodTypeDesc}, {@link MethodHandleDesc}, and
>  * {@link DynamicConstantDesc}; this list may be extended to reflect 
> future
>  * changes to the constant pool format as defined in JVMS 4.4.
>
> The view factories seem to have a naming inconsistency; there is 
> `asPinned` and `asReadonly`, but `slice`.  Given the slightly odd 
> treatment of views when it comes to temporal bounds, I would prefer 
> the asXxx (which suggests you're going to use the view in place of the 
> original) form, and suggest `asSlice` or `asNarrowed`.
Good point - yes, the slice is a view and all views should use uniform 
naming.
>
> I think you also want to give the sealing treatment to PathElement.

Yup

Thanks
Maurizio

>
>
>
> On 9/24/2019 10:06 AM, Maurizio Cimadamore wrote:
>> Hi,
>> as the subject says, the implementation classes of the layout API do 
>> not always store their properties into final fields, and they resort 
>> to lazy computation, etc. This negatively impacts C2 scrutability of 
>> same data structures.
>>
>> This patch fixes this situation, by changing size/alignment to be 
>> final fields in AbstractLayout - so that they will have to be 
>> provided before hand. I've added, for clarity, and extra 'default' 
>> constructor to all layout implementation classes which allows to 
>> create a layout with standard alignment and empty name.
>>
>> There are also few minor changes:
>>
>> * I've tweaked VM to also trust final fields in the layout package
>>
>> * I've rearranged some some scope classes so that their creation is 
>> less straightforward, more transparent and requires less reflective 
>> checks. This is particularly evident in HeapScope and BufferScope. 
>> Note that I also changed the public API of MemorySegment::ofArray and 
>> replaced that with multiple overloads (one per primitive array). This 
>> is good because it makes the code more 'static' and also because it 
>> removes the possibility for the user to pass in a wrong array type.
>>
>> * I've re-ordered the way in which scope vs. segment is created - 
>> that is, instead of this
>>
>> new XYZSegment(.., ..., ..., new XYZScope(...))
>>
>> We now do this:
>>
>> XYZScope scope = new XYZScope(...);
>> new XYZSegment(.., ..., ..., scope)
>>
>> As this makes a difference for C2 (Vlad pointed this out).
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8231402_v2/
>>
>>
>> With this patch, the level of performances of the memory access API 
>> is virtually on par with Unsafe in our suite of synthetic benchmarks, 
>> at least when using the Graal compiler (*). With C2 there are still 
>> issues which have to do mainly with (i) escape analysis not being 
>> aggressive enough (a VM patch is required) and (ii) inlining not 
>> working well in relatively 'cold' code (e.g. segment 
>> creation/closure), so that some manual sprinkling of @ForceInline 
>> annotations is  required. I will pursue these in a follow up patch.
>>
>> Maurizio
>>
>> (*) the only exception to this is a test which performs indexed 
>> access, in which Graal compiler is not able to vectorize the loop 
>> when using the memory access API (because of the presence of address 
>> operation on longs); that said, performance of code compiled by the 
>> Graal compiler with the memory access API is still superior than that 
>> of C2 using unsafe (I'm also following up with the Graal compiler 
>> team on this issue). There also seems to be an issue with the 
>> liveness check which is never hoisted out of hot loops - leading to 
>> slightly slower performances; in C2 we have a similar issue and it's 
>> caused by the fact that the VM puts a memory barrier after Unsafe 
>> memory access calls (since the Unsafe call could touch the loop 
>> invariant itself!). We obviously need to relax some of these checks 
>> if the Unsafe call occurs from within the memory access API (which 
>> does not allow arbitrary read/write of Java fields).
>>
>>
>>
>