[foreign-memaccess] 8227393: Add Constable support to the layout API

[Jorn Vernee]

Thanks for the response.

Like you say it's more of a future issue, but I wanted to bring it up 
since it might play into our decisions now :) (and I'm always curious to 
hear other people's thoughts).

The patch looks good to me.

Jorn

On 2019-07-09 20:03, Maurizio Cimadamore wrote:
> Actually, I disagree. Once created, a Layout endianness is part of the
> layout itself. So, the underlying CP representation should just
> reflect whatever is in the layout.
> 
> The problem you raise is more related to the fact that an assumption
> made at compile-time (e.g. on endianness) can be invalidated at
> runtime, but that's more an issue that has to be addressed when (and
> if) javac support this. For instance, when investigating
> intrinsification opportunities for String.format a similar problem
> arose: the Locale used to format the string was not guaranteed to be
> preserved from compile- to run- time. The solution was to come up with
> a BSM which check the compile-time assumptions against the run-time
> ones and fallback to usual code when such invariants were violated.
> 
> So, there are two issues: one is to be able to embed layout constants
> in the constant pool, which is addressed by this patch. A second (and
> arguably deeper issues) is how to reliably intrinsify layouts, so that
> we don't accidentally depend on conditions that were present at
> compile-time, but are invalidated at run-time. To me these are
> separate issues.
> 
> I also think that at some point we will need to address the stance on
> endianness - right now we expose some 'endianness-inferring' factories
> which are, I think, misleading. I think a better approach would be for
> the factories to be always explicit, and then to have a set of
> constants which users can import, which 'do the right thing'
> (depending on what 'the right' thing is in a given context).
> 
> Also, we have the option of, if we wanted to, keep track if an
> endianness-inferring factory was used and, if so, simply refuse to
> return a ConstantDesc. This means that we would only be able to create
> ConstantDesc for things that have an explicitly provided endianness,
> which would then side-step the issues you mentioned.
> 
> Maurizio
> 
> On 09/07/2019 18:01, Jorn Vernee wrote:
>> Looking at this again, I realized there's a problematic part w.r.t. 
>> ByteOrder and compile time constant folding (like condy-folding) that 
>> we might want to do later on. Let's say we compile on an LE system, 
>> and we're creating a native byte order ValueLayout. javac creates a 
>> trackable constant value, and then commits this to the constant pool 
>> using describeConstable, and it's going to be explicitly LE, instead 
>> of native order. When loading this constant on a system that is Big 
>> Endian, we are gonna get a ValueLayout that is supposed to be native 
>> byte order, i.e. BE, but is actually LE.
>> 
>> I think one way to get rid of that discrepancy is to modify 
>> java.nio.ByteOrder to be Constable also, and to have a flag for 
>> tracking if it's native order, then take this flag into account when 
>> creating a ConstantDesc to make sure to call `nativeOrder()` instead 
>> of loading the LE or BE constant directly.
>> 
>> Another way is to track if it's native byte order in ValueLayout 
>> itself, and choose a different bootstrap appropriately.
>> 
>> Thoughts on this?
>> 
>> Thanks,
>> Jorn
>> 
>> On 2019-07-09 17:01, Maurizio Cimadamore wrote:
>>> Updated webrev:
>>> 
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8227393_v2/
>>> 
>>> (addressed all comments, including covariant overrides)
>>> 
>>> Thanks
>>> Maurizio
>>> 
>>> On 08/07/2019 16:22, Jorn Vernee wrote:
>>>> Hi,
>>>> 
>>>> Some comments:
>>>> 
>>>> - The constants added to AbstractLayout should probably be `final`? 
>>>> (Also minor naming; these use `XXX_CD` while ConstantDescs uses 
>>>> `CD_XXX`, might want to follow the latter)
>>>> - In MemoryHandles.java there are 2 imports added that are not used.
>>>> - In TestLayoutConstants.java, there seems to be a spurious line: 
>>>> `MemoryLayout.ofStruct(MemoryLayout.ofFloatingPoint(32), 
>>>> MemoryLayout.ofPadding(8));`.
>>>> 
>>>> Also, what do we want to do with covariant return types for various 
>>>> `describeConstable` methods? e.g. GroupLayout::describeConstable 
>>>> could return Optional<DynamicConstantDesc<GroupLayout>>, do we want 
>>>> to go that route?
>>>> 
>>>> Cheers,
>>>> Jorn
>>>> 
>>>> On 2019-07-08 16:17, Maurizio Cimadamore wrote:
>>>>> As suggested by Brian, there's no reason as to why MemoryLayout
>>>>> classes could not implement the Constable interface. After all, 
>>>>> they
>>>>> are static description of some memory layout, and a constant pool 
>>>>> form
>>>>> for them can be defined (using condy). This patch does just that.
>>>>> 
>>>>> Webrev:
>>>>> 
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8227393/
>>>>> 
>>>>> Now, there are some degree of freedom in doing this mapping; from
>>>>> other cases, I infer that the idiomatic way to implement constable
>>>>> would be to return a custom DynamicConstantDesc (e.g. enums return 
>>>>> an
>>>>> EnumDesc, VarHandles a VarHandleDesc) - but in this case I'm not 
>>>>> sure
>>>>> the exercise is worth it for two reasons:
>>>>> 
>>>>> * I can't see any 'interesting method' that could be added to a
>>>>> LayoutDesc - seems simpler to just work on the layouts themselves
>>>>> 
>>>>> * To canonicalize dynamic constants representing layouts, we'd 
>>>>> still
>>>>> need some behavioral changes in the public SE API - which is a bit
>>>>> tricky given everything is defined in an incubator module
>>>>> 
>>>>> I'd say that what I have implemented is good enough for now, but of
>>>>> course we should reserve the right to change it later if/when the 
>>>>> code
>>>>> ends up in java.base.
>>>>> 
>>>>> Similar considerations hold for memory access VarHandles Constable
>>>>> support - on the one hand we could just add such support - on the
>>>>> other hand, to do that correctly we'd need some surgery in
>>>>> VarHandleDesc - which is again a SE API in java.base. I think when 
>>>>> the
>>>>> time comes, we'd add a new 'kind' to VarHandleDesc called MEMORY, 
>>>>> and
>>>>> then add a suitable implementation of describeConstable under
>>>>> VarHandleMemoryAddressBase (as well as canonicalize support).
>>>>> 
>>>>> Note: I had to add a 'missing' BSM constant in ConstantDescs - 
>>>>> namely
>>>>> for ConstantBootstraps#getStaticFinal. I hope this change is
>>>>> innocuous/standalone enough.
>>>>> 
>>>>> Cheers
>>>>> Maurizio