[lworld] RFR: 8334484: [lworld] new translation strategy for instance field initializers [v3]

[Maurizio Cimadamore]

On Sat, 29 Mar 2025 01:06:05 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Proxy locals are added as a new compiler phase which is executed just before code generation.
>> 
>> Q - what it does?
>> A - basically for code like:
>> 
>> abstract value class Super {
>>     Super(String s) {}
>> }
>> 
>> value class V extends Super {
>>     int i;
>>     V(String s) {
>>         if (s != null) {
>>             i = 100;
>>         } else {
>>             i = 200;
>>         }
>>         super("100" + s);
>>     }
>> }
>> 
>> javac will generate for V's constructor something in the lines of:
>> 
>>     V(String s) {
>>         /*synthetic*/ int local$i;
>>         if (s != null) {
>>             local$i = 100;
>>         } else {
>>             local$i = 200;
>>         }
>>         {
>>             /*synthetic*/ final String local$0 = "100" + s;
>>             i = local$i;
>>             super(local$0);
>>         }
>>     }
>> 
>> so given a constructor for which any strict field is assigned to more than once, javac will generate synthetic local variables that will correspond to it and any read or write done to this strict field will be done on the synthetic local variable.
>> 
>> Javac will also generate additional synthetic local variables to hold the arguments, if any, of the super constructor invocation and will assign the strict fields with the current value of the corresponding synthetic local just before invoking the super constructor.
>> 
>> Q - does it interacts with other valhalla features?
>> A - yes it has a direct interaction with the new stackmap table as the new assert_unset_fields entry, in most cases, is not generated now
>> Q- is it on by default?
>> A- yes but there is a hidden option to tell javac not to generate local proxy variables: `noLocalProxyVars` this will allow us to continue using javac for test cases that check for the generation of the assert_unset_fields entry in the stackmap table attribute until we have time to migrate those tests
>> 
>> TIA for any comments
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressing review comments

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LocalProxyVarsGen.java line 151:

> 149: 
> 150:     public void visitMethodDef(JCMethodDecl tree) {
> 151:         if (strictFieldsReadInPrologue.get(tree) != null) {

Should this call `remove`, so that, as we visit, we get rid of entries (and keep memory usage under control) ?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LocalProxyVarsGen.java line 153:

> 151:         if (strictFieldsReadInPrologue.get(tree) != null) {
> 152:             Set<Symbol> fieldSet = strictFieldsReadInPrologue.get(tree);
> 153:             if (!strictInstanceFields.isEmpty()) {

Do we need this code? After all, we know from `Resolve` that a strict instance field was accessed -- so what is left to verify here?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LocalProxyVarsGen.java line 200:

> 198:         }
> 199:         constructor.body.stats = localDeclarations.toList();
> 200:         if (!assigmentsBeforeSuper.isEmpty()) {

Can this ever be empty (given we end up here only when `Resolve` found something) ?

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1403#discussion_r2021025485
PR Review Comment: https://git.openjdk.org/valhalla/pull/1403#discussion_r2021027783
PR Review Comment: https://git.openjdk.org/valhalla/pull/1403#discussion_r2021030349