Brian, I really like where this is going. Our software is full of totally encapsulated (thus, non-record) and yet truly immutable classes that would greatly benefit from additional optimizations that are possible if final fields are trusted. If it turns out to be infeasible to figure out which final fields can/cannot be trusted in the legacy code, I really hope that, at least, we will have an option to designate newly developed classes with final fields to be trusted in a way that is not tied to records. I believe that a significant fraction of Java community can share similar accounts.
Sincerely, Roman Elizarov On Fri, Nov 13, 2020 at 6:51 PM Brian Goetz <[email protected]> wrote: > 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 > <[email protected]> <[email protected]> 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. > > > >
