Convert to using public JDK API #618
Conversation
There was a problem hiding this comment.
Thank you for this contribution @Arthurm1! This PR represents a lot of work and the changes are a super valuable improvement. I'm very much in favor of merging this PR.
- It's OK to update the snapshots for the failing tests. I can execute
snapshots/runlocally and push to your branch if there are problems running that command on Windows. - The only blocking comment is about
getOverriddenMethods.
| @@ -20,21 +19,21 @@ enum Enums { | |||
| A("A", 420), | |||
| //^ definition semanticdb maven . . minimized/Enums#A. | |||
| // documentation ```java\nEnums.A("A", 420) /* ordinal 0 */\n``` | |||
| //^ reference semanticdb maven . . minimized/Enums#`<init>`(). | |||
| @@ -61,6 +61,7 @@ public static String app() { | |||
| // ^^^^^^^^^^^ reference semanticdb maven . . minimized/Fields#InnerFields# | |||
| // ^^^^^^^^^^^ definition local 1 | |||
| // documentation ```java\nInnerFields innerFields\n``` | |||
| // ^^^^^^ reference local 0 | |||
| @@ -41,12 +41,15 @@ public enum InnerEnum { | |||
| A, | |||
| // ^ definition semanticdb maven . . minimized/InnerClasses#InnerEnum#A. | |||
| // documentation ```java\nInnerEnum.A /* ordinal 0 */\n``` | |||
| // ^ reference semanticdb maven . . minimized/InnerClasses#InnerEnum#`<init>`(). | |||
There was a problem hiding this comment.
While this is technically correct, the navigation here might be confusing since the constructor is auto-generated. I'm OK with merging this PR with this change, but it would be nice to follow up with an issue or separate PR to undo this change.
| @@ -167,9 +170,6 @@ public static void testEnum(InnerEnum magicEnum) { | |||
| // ^^^^^^^^ definition semanticdb maven . . minimized/InnerClasses#testEnum(). | |||
| // documentation ```java\npublic static void testEnum(InnerEnum magicEnum)\n``` | |||
| // ^^^^^^^^^ reference semanticdb maven . . minimized/InnerClasses#InnerEnum# | |||
| // ^^^^^^^^^ reference semanticdb maven . . minimized/InnerClasses#InnerEnum#`<init>`(). | |||
| // documentation ```java\nfinal String message\n``` | ||
| // ^^^^^^^ definition local 1 | ||
| // documentation ```java\nfinal String message\n``` | ||
| // ^^^^^^^ definition semanticdb maven . . minimized/Hello#HelloBuilder#message(). |
| @@ -18,6 +18,7 @@ public static void main(String[] args) { | |||
| TypeVariables.app(new TypeVariables.CT()); | |||
| // ^^^^^^^^^^^^^ reference semanticdb maven . . minimized/TypeVariables# | |||
| // ^^^ reference semanticdb maven . . minimized/TypeVariables#app(). | |||
| // ^^^^^^^^^^^^^ reference semanticdb maven . . minimized/TypeVariables# | |||
| @@ -46,6 +47,8 @@ public static void main(String[] args) { | |||
| // ^^^ reference semanticdb maven . . minimized/Primitives#app(). | |||
| + new ParameterizedTypes<Integer, String>().app(42, "42") | |||
| // ^^^^^^^^^^^^^^^^^^ reference semanticdb maven . . minimized/ParameterizedTypes#`<init>`(). | |||
| // ^^^^^^^ reference semanticdb maven jdk 11 java/lang/Integer# | |||
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/GlobalSymbolsCache.java
Show resolved
Hide resolved
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.java
Show resolved
Hide resolved
|
@olafurpg I've reverted the I've also managed to run the tests under WSL. I guess I could fix the enums This should be good enough to merge though. I've highlighted internal API usage with warning comments. Is it possible to test the performance of the |
|
Thank you @Arthurm1 ! The repo has JMH benchmarks that we can use to confirm performance claims. I will review the changes tomorrow and share instructions on how to run the benchmarks. |
There was a problem hiding this comment.
LGTM 👍🏻 Thank you @Arthurm1 !
I opened #621 to get the benchmarks running again. The following command should target only the relevant benchmarks to measure overhead of the compiler plugin
bench/jmh:run -i 10 -wi 10 -f1 -t1 -p lib=guava .*.CompileBench
You can reduce the number of iterations from 10 to a smaller number like 5, but you may not get as accurate numbers then. I suspect the performance difference is negligible when using your reimplementation of overrides computation, but it's nice to at least confirm it with actual benchmarks instead of relying on intuition.
|
To fix the override suite, you need to manually update the string literal in the assertion. There's no automatic way to update that snapshot |
|
Using the public API is about 3% slower if I'm reading these results right? I'm no expert on benchmarking Private API Public API You should be able to merge this PR now as tests pass |
|
I think I'll have to look at performance some more. The above figures were just for when swapping out the The figures for public API vs private API are... Public... Private... So Private API adds 25% to compile time and Public API adds 67% |
|
Reworked to cache all Trees in a Map until full scan is complete. Then tree references can be looked up directly in the Map (which caused a lot of the performance drop off). Now performance is on par with current release... |
|
Hey @Arthurm1 sorry for radio silence on this PR - I will take a look and re-run benchmarks just to reproduce what you are seeing. Exciting times! |
|
@keynmol No worries. I've got a final commit that I want to push through - changing a |
|
@keynmol Sorry - accidentally included some notes so pushed twice All done I think - you should be able to test performance. |
|
@Arthurm1 This morning I ran the benchmarks on my linux machine, glanced at them, put them in a file and... left the house without pasting the results 🤦 From my quick glance I could confirm the performance was on part over 10 warmup iterations and 10 iterations. I'll review the PR with that in mind, and will paste the results later today to make sure there's no regression. |
|
@keynmol doh. Just merge it - what's the worst that could happen 😉 |
|
I finally got back to that machine - benchmarks look good! I'll take one more look tomorrow morning and we'll merge and release this |
There was a problem hiding this comment.
I'm sorry for going completely silent for so long 🙇🏻
Thank you @Arthurm1 for closing the performance gap. It's great that we are able to move to the public API without performance penalties. This PR is a huge contribution 🙏🏻 I really appreciate your work on this. I'll let Anton do a last round of review before we merge. LGTM 👍🏻
There was a problem hiding this comment.
Amazing effort!
I ran benchmarks again in a codespace:
Benchmarks - on Arthur's PR
[info] Benchmark (lib) Mode Cnt Score Error Units
[info] CompileBench.compile bytebuddy ss 10 2523.846 ± 75.302 ms/op
[info] CompileBench.compile guava ss 10 3288.573 ± 85.234 ms/op
[info] CompileBench.compileSemanticdb bytebuddy ss 10 3227.423 ± 78.047 ms/op
[info] CompileBench.compileSemanticdb guava ss 10 4270.741 ± 119.716 ms/op
[info] ScipSemanticdbBench.json N/A ss 10 2311.662 ± 48.781 ms/op
[info] ScipSemanticdbBench.jsonParallel N/A ss 10 1280.459 ± 61.906 ms/op
On main
[info] CompileBench.compile bytebuddy ss 10 2565.034 ± 81.598 ms/op
[info] CompileBench.compile guava ss 10 3456.703 ± 87.916 ms/op
[info] CompileBench.compileSemanticdb bytebuddy ss 10 3149.586 ± 79.741 ms/op
[info] CompileBench.compileSemanticdb guava ss 10 4406.520 ± 134.021 ms/op
[info] ScipSemanticdbBench.json N/A ss 10 2750.678 ± 76.973 ms/op
[info] ScipSemanticdbBench.jsonParallel N/A ss 10 1495.191 ± 54.868 ms/op
Thanks to the work in sourcegraph/scip-java#618 these are no longer necessary!
This is an attempt to use the JDK public API instead of the private one.
The advantage is that for JDK 17+ there should be (mostly) no need for
--add-exportswhich should make it easier for tools that use this plugin e.g. Metals.I can't test this completely locally as I'm using Windows and some tests won't run.
I'm also not entirely sure what the outputs should be. In some cases they look better e.g.
enumsand for some parameterised types.For the library tests there are a lot of differences but the correct packages seem to now be displayed in test results instead of
_root_.I have no idea how this affects performance.
Types,TreesandElementsnow sometimes have to be queried to lookup data instead of being able to query the internal classes directly so maybe slower 🤷I've changed the
-targetroot:javac-classes-directoryoption to use reflection so--add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMEDonly has to be supplied if that option is specified.Test plan
Rerun all the tests I guess