The Record Attribute - What does it mean to be a record at runtime?

[Brian Goetz]

Taking a step back here ...

I agree with pretty much everything Maurizio says about the interaction 
of records and trusted fields.

But, I think we should also recognize that we fell, somewhat, for a 
siren song.  We all hate the fact that final fields are not really 
final.  So when another opportunity came along to strike a blow for a 
rational memory model, we were all over it. But perhaps we swung at the 
wrong pitch.

It is easy to agree "yes, those fields should be trusted."  But the 
amount of work we had to do to support just this one case -- including 
JIT, reflection, unsafe, and var handles -- was already a lot.  Then we 
found out that we didn't even do it right, and we had to have this 
discussion about how to remediate it, and more work to fix it.  
Reclaiming a sane memory model 1% at a time does not seem to be an 
expensive and slow path.

While it is easy to say, Remi's comment that we are focusing on the 
wrong side of the timeline seems the right way to start this 
conversation -- how do we get to the point where it is simple to add new 
classes to the set which are immune to to such shenanigans.

In part, that means gathering some requirements as to when to _not_ 
trust final fields.  Has anyone characterized the space where this is 
required?



On 11/12/2020 11:34 PM, Dan Smith wrote:
>> On Nov 12, 2020, at 2:15 PM, Maurizio Cimadamore <Maurizio.Cimadamore at oracle.com> wrote:
>>
>> I think the current state of affair for records has at least 3 issues:
>>
>> * The paths which controls which fields are trusted by VM and which fields are exposed via reflection/292/unsafe do not align 100%. For instance, Unsafe refuses to give up field offset based on the stricter Class::isRecord definition, thus creating a safety gap.
> Agree that's a problem. All the components involved in trusted fields need to be in agreement about which final fields are trusted.
>
>> * In an attempt to prevent changes to records fields via reflection, the specification of Field::set was updated to say that if the field holder is a record, the field update will fail. This is good, if it weren't that the javadoc has to define what it means for a field holder to be a record; and the way that's done is by referring to Class::isRecord, which is not what the implementation does (the implementation uses the broader "isRecord" definition based on the presence/absence of the record attribute - the same used by the VM).
> Agree the spec and implementation should be consistent. That doesn't necessarily mean they need to use Class.isRecord as their test. It could just as easily be checking for the attribute, or testing that the class extends java.lang.Record, or maybe something else. There are multiple places to draw the line.
>
> But all I'm saying is that there are multiple possibilities. While I think having a super-simple test might be attractive, I'm not in a position to weigh that against other concerns.
>
>> * All clients have now a way to enable the (fragile) final field optimization: generate a class, then attach a record attribute to it (e.g. with ASM). The VM will trust all fields in that class no matter what. I think this is probably not what was originally intended by the changes in JDK-8247444. In other words, we now have a mechanism which is kind of like the internal @ForceInline annotation - not as handy as an annotation perhaps, but much more public (because, any classfile can be augmented to contain an extra, maybe empty, record attribute).
> Right. If there's a concern that *too many* classes will have trusted fields, then drawing the line more narrowly is useful. It's possible—I'm not sure—that saying "all subclasses of java.lang.Record" would raise fewer concerns here than "all classes with a Record attribute". Not because it's necessarily harder to physically put in the class file, but because extending a class is a strong signal that you intend to adopt any special contracts/treatment associated with that class.
>