search

eisop / checker-framework

Pluggable type-checking for Java
https://eisop.github.io/
Other
18 stars 18 forks source link

Conservatively remove `InitializationStore#newFieldValueAfterMethodCall` to optimize performance #712

Closed wmdietl closed 7 months ago

wmdietl commented 7 months ago

@flo2702 This file was changed as part of #444.

If I change the method body to return true there is only one test failure: checker/tests/initialization/FlowFbc.java.

Some rough timing:

Just returning true in isDeclaredInitialized: Issue1438.java: ~29s Issue1438b.java: ~56s Issue1438c.java: ~12s

Simplified implementation of isDeclaredInitialized: Issue1438.java: ~44s Issue1438b.java: ~55s Issue1438c.java: ~14s

EISOP before this PR: Issue1438.java: ~42s Issue1438b.java: ~55s Issue1438c.java: ~41s

EISOP before #444 was merged: Issue1438.java: ~18s Issue1438b.java: ~36s Issue1438c.java: ~11s

With Issue1438c there is a significant performance improvement with the simplified implementation, nearly the same as returning true. Issue1438.java and Issue1438b.java are unchanged by this PR, whereas returning true only speeds up Issue1438.java.

@flo2702 Can you see whether the simplified implementation is unsound? Anything you see that could be improved? All nullness tests pass, but there are not many tests with NotOnlyInitialized. Can you think of a reason why static field accesses and instance field accesses in constructors are not sped up by this PR? As instance fields in instance methods seem the most frequent use, this PR might still be a good improvement.

Below is text before I added Issue1438c.java: There is some speedup for static fields, but no change for instance fields, from this PR... which is a bit odd. The large discrepancy between static and instance fields, even before this PR, is also troubling.

@flo2702 Can you have a look at the performance numbers? I think I see isDeclaredInitialized on some performance charts, but maybe that is not actually the performance culprit.

If isDeclaredInitialized and sound handling of NotOnlyInitialized were the performance problem, we could add a new option to disable sound handling of NotOnlyInitialized, if that improves performance. (Or unsound behavior by default, but a warning when NotOnlyInitialized is seen.)

wmdietl commented 7 months ago

@flo2702 I indeed confused myself with the different versions of the benchmark. I now added Issue1438c.java that tests instance fields accessed from an instance method. There is a significant speedup for this common case. Please let me know what you think of this change. I edited the PR description with updated timing.

wmdietl commented 7 months ago

@flo2702 Thanks for looking into this! I tried your fieldAccess.getField().getAnnotation(Initialized.class) != null proposal: it didn't change timing and I think it always returns false. So I went with the conservative answer to just return false and simply commented out this logic. Final timing for me:

Issue1438.java: ~19s Issue1438b.java: ~55s Issue1438c.java: ~12s

I've filed #720 to add more tests for NotOnlyInitialized and see whether this is too conservative.

719 for additional performance improvements.