Configure Compiler to optimize out unused local variables

[HannesWell]

Fixes eclipse-platform/eclipse.platform.releng.aggregator#1653

[HannesWell]

First pass looks good, couldn't find the baseline difference warning for o.e.core.jobs anymore mentioned in eclipse-platform/eclipse.platform.releng.aggregator#1653 (comment).

[github-actions]

Test Results

     591 files  ±0       591 suites  ±0   1h 3m 16s ⏱️ + 3m 6s
  3 845 tests ±0    3 837 ✔️ +1    8 💤 ±0  0 ❌  - 1 
12 141 runs  ±0  12 096 ✔️ +1  45 💤 ±0  0 ❌  - 1 

Results for commit 6539b58. ± Comparison against base commit a5789fb.

♻️ This comment has been updated with latest results.

[Bananeweizen]

I fully support that change. However, at work I got complaints when using it and finally had to revert it. There are lots of people who are used to saving files at arbitrary times instead of using automated saving on launch, and they are totally confused if their (not yet used) just declared variables are gone again. So please prepare for some complaints. :)

[HannesWell]

I fully support that change. However, at work I got complaints when using it and finally had to revert it. There are lots of people who are used to saving files at arbitrary times instead of using automated saving on launch, and they are totally confused if their (not yet used) just declared variables are gone again. So please prepare for some complaints. :)

Thanks for the heads-up. But isn't it the default to be asked to save dirty files before launch? Why would one not like to do that?

But aside from that thinking about it again I'm not sure anymore if this preference really fixes the comparator error or if I just implicitly enforce a qualifier update for all the affected bundles?
Nevertheless I think the final built jars should have unused variables definitively removed.

[laeubi]

I think the problem is more if you add a local variable and then set a breakpoint there you might be surprised if the debugger says the variable is not found ;-)

[HannesWell]

But aside from that thinking about it again I'm not sure anymore if this preference really fixes the comparator error or if I just implicitly enforce a qualifier update for all the affected bundles?
Nevertheless I think the final built jars should have unused variables definitively removed.

Comparing the content of the class_file generated for the ImplicitJobs class using the ByteCode view indeed shows a difference when the preference is changed (after recompilation).
And if I compare the generated class file with unused variables being optimized out it is equal (at least the
MD5, SHA1 and SHA256 hash-sum) to the class file from I20231207-1800.
With that I have the impression that this is a real fix.

But there are still a few baseline and build artifacts have same version but different contents messages left, will have a look.

I think the problem is more if you add a local variable and then set a breakpoint there you might be surprised if the debugger says the variable is not found ;-)

Mhm, yes. This is indeed sub-optimal.
Probably this setting should just not be the same for the IDE and the build?!

[HannesWell]

I think the problem is more if you add a local variable and then set a breakpoint there you might be surprised if the debugger says the variable is not found ;-)

Mhm, yes. This is indeed sub-optimal. Probably this setting should just not be the same for the IDE and the build?!

Can we overwrite it again in the build-settings?

[laeubi]

Comparing the content of the class_file generated for the ImplicitJobs class using the ByteCode view indeed shows a difference when the preference is changed (after recompilation).
And if I compare the generated class file with unused variables being optimized out it is equal (at least the
MD5, SHA1 and SHA256 hash-sum) to the class file from I20231207-1800.
With that I have the impression that this is a real fix.

That feel suspicious to me ... just assuming that we don't have any unused variables around in the code it should not make a difference.

[HannesWell]

Comparing the content of the class_file generated for the ImplicitJobs class using the ByteCode view indeed shows a difference when the preference is changed (after recompilation).
And if I compare the generated class file with unused variables being optimized out it is equal (at least the
MD5, SHA1 and SHA256 hash-sum) to the class file from I20231207-1800.
With that I have the impression that this is a real fix.

That feel suspicious to me ... just assuming that we don't have any unused variables around in the code it should not make a difference.

That's right and the ImplicitJobs class for which I verified it does not seem to have unused variables (at least there are no warnings and SonarLint also does not complain)

[laeubi]

It might be that something like this:

String s = "...";
callMethod(s);

will be transformer by the compiler to

callMethod("...");

because s is "unused", but this will of course then be not so nice when debugging the code, but that's just a guess.

[HannesWell]

Could be possible. But I have no clue. @iloveeclipse can you tell?
But on the other hand this would also mean that eclipse bundles consumed through the TP would be far less debuggable, so I'm not sure this is really the case..

Anyway with the things said, I more and more have the impression that the same value is not suitable for build-time and IDE.
I think both should stay as it was. In the build unusedLocal=optimize out and in the IDE unusedLocal=preserve. Which gives me the question if that specific project preference can be overwritten in the tycho-compiler plugin?
Looking at https://tycho.eclipseprojects.io/doc/4.0.4/tycho-compiler-plugin/compile-mojo.html I only see compilerArgs, but I don't know if this takes precedence over project-setting and how to use it.

[szarnekow]

I’d rather prefer a validation that flags unused local cars with a warning but keeps them in bytecode. Different compilation artifacts in IDE and in the build ask for confusion. Not seeing some local variables when debugging code that was consumed from the TP isn’t nice either.

[iloveeclipse]

I would revert original PR before starting to change compilation settings everywhere without understanding of all possible side effects.

I have no clue about this particular option but I see it is "preserving" by default and I would trust original compiler authors who enabled that.

Note: your change succeeds most likely not because of compiler settings effect on the bytecode but because every affected bundle is considered as changed now with preferences files changed.

[iloveeclipse]

@srikanth-sankaran or @jarthana : could you please help here to understand for what org.eclipse.jdt.core.compiler.codegen.unusedLocal=preserve is good for and what would be the consequences of changing that to optimize out.

[HannesWell]

I’d rather prefer a validation that flags unused local cars with a warning but keeps them in bytecode. Different compilation artifacts in IDE and in the build ask for confusion. Not seeing some local variables when debugging code that was consumed from the TP isn’t nice either.

Agree on that, but as it was discussed above that preference seem to do more than only removing unused variables since the class I verified (ImplicitJobs) has no unused variables.

Note: your change succeeds most likely not because of compiler settings effect on the bytecode but because every affected bundle is considered as changed now with preferences files changed.

As written above, I suspected the same first too, but when comparing the generated class file for ImplicitJobs with optimize-out and perverse it showed a difference. And furthermore the class file generated in the build three days ago generated a class file equal to the one generated by the IDE with setting optimize-out.
So by default the build currently uses optimize-out.

[vogella]

Sorry if that was discussed already but you could also use the JDT clean-up to remove unused local variables

[HannesWell]

Sorry if that was discussed already but you could also use the JDT clean-up to remove unused local variables

Most of the affected classes probably don't have unused local variables, nevertheless the byte-code seems still different with this option set on preserve. So the question is if we want to keep that or not since the default in the build (not in the IDE) until now seems to have been optimize out.

So yes of course it would be best to have no unused variables in the code eventually, but for the problem it does not make a difference.

[jukzi]

I don't want to use "optimize out". For sure there is a reason that the ui default is "preserve". But i suggest to apply this change to see if it solves eclipse-platform/eclipse.platform.releng.aggregator#1653 (comment)

We could then later revert this PR if we get in trouble with "optimize out". what i do not understand: why that option changes the output of for example org.eclipse.core.internal.jobs.ImplicitJobs - that class contains no marker for unused variables!

[jukzi]

@akurtakov submit and start new i build, please?

[akurtakov]

I don't feel comfortable pushing this one for the sake of testing if the plan is to revert it shortly after.
If there was a bundle with git change it will stay with the "optimized out" build in the I-builds repo until touched again.
@iloveeclipse wdyt?

[iloveeclipse]

I highly dislike that because some projects already use project settings. At the end i would prefer that all projects use projects settings.

I didn't said aggregator build should NOT use the settings, all what I want is that we have a clear understanding for entire aggregator build what we are doing, which settings are used and that this is applied consistently for all projects participating in aggregator build.

[akurtakov]

3. I think we should revert original PR (or only one part of it that overrides aggregator setting) that caused troubles and came up with a vision/solution for entire aggregator build, not just parts of it.

I highly dislike that because some projects already use project settings. At the end i would prefer that all projects use projects settings.

Can you point to such a project?

enable useProjectSetting=true for single known to fail bundle (so no unexpected changes go in) to verify this is the actual fix, if it fails revert and forcequalifierupdate for this bundle only.

Sounds better, i however don't know how todo that.

I'll execute step 1 (revert the eclipse.platform pom to configure compiler to use project settings now to unblock the build.

[srikanth-sankaran]

@srikanth-sankaran or @jarthana : could you please help here to understand for what org.eclipse.jdt.core.compiler.codegen.unusedLocal=preserve is good for and what would be the consequences of changing that to optimize out.

I would be very curious to see what changes are caused in the class file by this - As others have surmised, this option persists variables in the local variable table LocalVariableTable that would have been eliminated otherwise (due to never being read) - so that in a debugging session they are still available. There is not a whole lot more to it.

There is a single place where this state is consulted to drive class file generation: org.eclipse.jdt.internal.compiler.lookup.BlockScope.computeLocalVariablePositions(int, int, CodeStream)

The relevant block of code is:

			// check if variable is actually used, and may force it to be preserved
			boolean generateCurrentLocalVar = (local.useFlag > LocalVariableBinding.UNUSED && local.constant() == Constant.NotAConstant);

			// do not report fake used variable
			if (local.useFlag == LocalVariableBinding.UNUSED
				&& (local.declaration != null) // unused (and non secret) local
				&& ((local.declaration.bits & ASTNode.IsLocalDeclarationReachable) != 0)) { // declaration is reachable

				if (local.isCatchParameter()) {
					problemReporter().unusedExceptionParameter(local.declaration); // report unused catch arguments
				}
				else {
					problemReporter().unusedLocalVariable(local.declaration);
				}
			}

			// could be optimized out, but does need to preserve unread variables ?
			if (!generateCurrentLocalVar) {
				if (local.declaration != null && compilerOptions().preserveAllLocalVariables) {
					generateCurrentLocalVar = true; // force it to be preserved in the generated code
					if (local.useFlag == LocalVariableBinding.UNUSED)
						local.useFlag = LocalVariableBinding.USED;
				}
			}

			// allocate variable
			if (generateCurrentLocalVar) {

				if (local.declaration != null) {
					codeStream.record(local); // record user-defined local variables for attribute generation
				}
				// assign variable position
				local.resolvedPosition = this.offset;

				if ((TypeBinding.equalsEquals(local.type, TypeBinding.LONG)) || (TypeBinding.equalsEquals(local.type, TypeBinding.DOUBLE))) {
					this.offset += 2;
				} else {
					this.offset++;
				}
				if (this.offset > 0xFFFF) { // no more than 65535 words of locals
					problemReporter().noMoreAvailableSpaceForLocal(
						local,
						local.declaration == null ? (ASTNode)methodScope().referenceContext : local.declaration);
				}
			} else {
				local.resolvedPosition = -1; // not generated
			}

A variable being optimized out results in one less entry in the LVT. It can also - perhaps not too obviously - result in changes to byte code emitted (@iloveeclipse, In our discussion earlier today, I said it will not change the opcodes produced - that is incorrect). The VM has special instructions for local variables in positions 0,1,2,3 (aload_{0, 1, 2, 3}) vs a generic aload for everything else. So an earlier variable being persisted or not may influence the opcode used for a later variable.

(Among other such changes)

ImplicitJobs is mentioned as a file that results in different class file being produced when this option is flipped. Is there a textual diff linked to from somewhere ? I have heard of comparator errors in the past but have never worked on them to know where I can get a drill down view of textual delta produced by say running the disassembler.

[akurtakov]

@srikanth-sankaran You can use https://download.eclipse.org/eclipse/downloads/drops4/I20231210-2110/buildlogs/comparatorlogs/artifactcomparisons.zip to see the actual bytecode changes.

[akurtakov]

@srikanth-sankaran Do you happen to know what is javac's behavior by default? It is interesting to know what the more wider used javac compiler does and maybe even use same default.

[jukzi]

Can you point to such a project?

org.eclipse.jdt.annotation\pom.xml
org.eclipse.jdt.debug.jdi.tests\pom.xml
org.eclipse.jdt.ui\pom.xml
eclipse.platform\pom.xml
eclipse.platform\team\examples\org.eclipse.team.examples.filesystem\pom.xml
eclipse.platform.releng.aggregator\eclipse.jdt.core\org.eclipse.jdt.annotation\pom.xml
eclipse.platform.releng.aggregator\eclipse.jdt.debug\org.eclipse.jdt.debug.jdi.tests\pom.xml
eclipse.platform.releng.aggregator\eclipse.platform\team\examples\org.eclipse.team.examples.filesystem\pom.xml

[srikanth-sankaran]

@srikanth-sankaran Do you happen to know what is javac's behavior by default? It is interesting to know what the more wider used javac compiler does and maybe even use same default.

A simple experiment with this snippet:

public class X {
    public static void main(String [] args) {
        int xyz = 33;
        int pqr = 123;
        System.out.println(pqr);
    }
}

shows javac emitting both xyz and pqr while ECJ persisting only pqr unless org.eclipse.jdt.core.compiler.codegen.unusedLocal is set to preserve

[jukzi]

shows javac emitting both xyz and pqr while ECJ persisting only pqr unless org.eclipse.jdt.core.compiler.codegen.unusedLocal is set to preserve

I dislike if such optimization causes debug to fail, it may reduce .class file size, put for performance the hotspot compiler will ignore needless code anyway.

[akurtakov]

Can you point to such a project?

org.eclipse.jdt.annotation\pom.xml org.eclipse.jdt.ui\pom.xml seem to be the only 2 production bundles with such setting, everything else is parent, example, test and/or duplicate.

• o.e.jdt.annotation is practially annotations only and the setting seems to be added by mass change eclipse-jdt/eclipse.jdt.core@f24a77a so it looks more like an accident
• *o.e.jdt.ui has the preserve setting in the very first commit of this file eclipse-jdt/eclipse.jdt.ui@eb4465d#diff-66b9a49c598db0fa1d6d63b87f24f7a5e7fc7c08d623068276432ad22094ca9bR22

Unfortunately, the above analysis didn't help us at all in this case.

[srikanth-sankaran]

@srikanth-sankaran You can use https://download.eclipse.org/eclipse/downloads/drops4/I20231210-2110/buildlogs/comparatorlogs/artifactcomparisons.zip to see the actual bytecode changes.

Thanks @akurtakov I analyzed a single case: ImplicitJobs

void endJob(InternalJob lastJob) {
		final Thread currentThread = Thread.currentThread();
		IStatus error;
		synchronized (this) {
			ThreadJob threadJob = threadJobs.get(currentThread);
			if (threadJob == null) {
				if (lastJob.getRule() != null)
					notifyWaitingThreadJobs(lastJob);
				return;
			}
			String msg = "Worker thread ended job: " + lastJob + ", but still holds rule: " + threadJob; //$NON-NLS-1$ //$NON-NLS-2$
			error = new Status(IStatus.ERROR, JobManager.PI_JOBS, 1, msg, new IllegalStateException(msg));
			//end the thread job
			endThreadJob(threadJob, false, true);
		}
		try {
			RuntimeLog.log(error);
		} catch (RuntimeException e) {
			//failed to log, so print to console instead
			System.err.println(error.getMessage());
		}
	}

There is indeed an unused local in the picture which is the exception parameter e in the catch clause catch (RuntimeException e) {

In the baseline case it is being stored into the variable (which is likely persisted - difficult to tell because I don't know what tool produced this byte code view - it looks substantially different from ecj disassembler/javap output) but in the build case it is being popped because the exception parameter is eliminated

[srikanth-sankaran]

Here is a small test case that mimics what happens in ImplicitJobs

Try this once with org.eclipse.jdt.core.compiler.codegen.unusedLocal is set to preserve and once with optimize-out (sp?)

public class X {
    public static void main(String [] args) {
        try {
        	System.out.println("Hello");
        } catch (RuntimeException e) {
        	
        }
    } 
}

Code produced with org.eclipse.jdt.core.compiler.codegen.unusedLocal is set to preserve:

public static void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=2, args_size=1
         0: getstatic     #16                 // Field java/lang/System.out:Ljava/io/PrintStream;
         3: ldc           #22                 // String Hello
         5: invokevirtual #24                 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
         8: goto          12
        11: astore_1
        12: return

Code produced with optimize-out

 public static void main(java.lang.String[]);
    descriptor: ([Ljava/lang/String;)V
    flags: (0x0009) ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=1, args_size=1
         0: getstatic     #16                 // Field java/lang/System.out:Ljava/io/PrintStream;
         3: ldc           #22                 // String Hello
         5: invokevirtual #24                 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
         8: goto          12
        11: pop
        12: return

See the pop vs astore_1 - when the exception parameter is optimized out, the caught exception need not be stored into the local (it is optimized out, so can't be stored into!) - so the value is popped off of the runtime operand stack

[srikanth-sankaran]

There is a single place where this state is consulted to drive class file generation: org.eclipse.jdt.internal.compiler.lookup.BlockScope.computeLocalVariablePositions(int, int, CodeStream)

This is indeed true, but this single place sets some state namely local.resolvedPosition to either -1 (variable is optimized out) or a valid offset. This is then consulted in other places in this case while generating code for the catch block to see if the caught exception must be stored or discarded (popped)

This concludes my analysis of the change in ImplicitJobs - other changes are left as homework assignment to concerned engineers 😄😄😄

[srikanth-sankaran]

In the baseline case it is being stored into the variable (which is likely persisted - difficult to tell because I don't know what tool produced this byte code view - it looks substantially different from ecj disassembler/javap output) but in the build case it is being popped because the exception parameter is eliminated

I misspoke - In the baseline it is being popped and the in the build it is being stored. Rest of the analysis stays

[srikanth-sankaran]

JEP 456 - unnamed variables and patterns will avoid such issues.

[jukzi]

Would be nice if then unused "e" in catch would leed to Problem marker with a quick fix to "_"

[HannesWell]

There is indeed an unused local in the picture which is the exception parameter e in the catch clause catch (RuntimeException e) {

Thank you @srikanth-sankaran for the analysis and explanation. This totally makes sense, I just didn't consider that unused catched exception variables are indeed unused variables.
And yes there are very likely many places in the platform code that have unused catched exceptions.

JEP 456 - unnamed variables and patterns will avoid such issues.
Would be nice if then unused "e" in catch would leed to Problem marker with a quick fix to "_"

Agree.

Now that #975 has reverted the change and that it is not really desired to optimize out unused variables in the IDE, I think this change is obsolete and can be closed.

[merks]

This was really interesting and informative. Thanks everyone!