Fixes #2720: Use StackWalker on Java 9+ to create Locations

[j-baker]

Fixes #2720.

In terms of memory allocations, this reduces the overall memory
allocations of creating a Location by an order of magnitude in
Java 9, and as compared to Java 8.

The implementation is somewhat involved due to these desires:

• Minimize the amount of work done if the Location is never used. This
  is done by not converting the StackFrame into a StackTraceElement,
  instead wrapping in an intermediate type. The StackTraceElement conversion
  will only occur (internally) if the .getFileName() method is called, which it usually isn't.
• Ensure the implementation is still Serializable. This is ensured with
  a .writeReplace method.
• Minimize the number of allocations, which is basically an exercise in
  lambda caching.
• Ensuring the old mechanism still works on Java 8.
• MethodHandles are even lower overhead than the reflection, but AnimalSniffer does
  not like them since they don't exist in Android.

Presently on Java 9+, on a stack depth of 1000 the old mechanism will allocate
40kB of RAM per call. The new one will allocate 1.5kB of RAM per call,
which is a huge improvement. Even in a very basic JUnit test with no stack depth,
the difference is about 1.5kB of allocation vs 8kB.

This is still sadly not the 'close-to-no-overhead' solution I was
looking for. I therefore also added a system property that can be used
to fully disable Location creation. I'm aware that this is likely not
the right approach given Mockito has plugins and settings - mostly
looking for guidance here given I'm not sure what would be idiomatic
here.

Checklist

• Read the contributing guide
• PR should be motivated, i.e. what does it fix, why, and if relevant how
• If possible / relevant include an example in the description, that could help all readers
  including project members to get a better picture of the change
• Avoid other runtime dependencies
• Meaningful commit history ; intention is important please rebase your commit history so that each
  commit is meaningful and help the people that will explore a change in 2 years
• The pull request follows coding style
• Mention Fixes #<issue number> in the description if relevant
• At least one commit should mention Fixes #<issue number> if relevant

[TimvdLippe]

The CI build is failing because of auto-formatting issues. You can run ./gradlew spotlessApply to fix these automatically.

Overall a nice improvement! Mostly some minor comments, but overall looking solid already. Good work 😄

[j-baker]

Latest commit is green but the status checks are red. Not sure why, but I assume a click of retry will do the trick.

[TimvdLippe]

@j-baker The latest commit referenced in this PR is the one from #2729. The JUnit test uses Class.forName, which breaks on Java 8. See my comment for a suggestion on how to fix it.

[codecov-commenter]

Codecov Report

Merging #2723 (af8454e) into main (95b43e5) will increase coverage by 0.01%.
The diff coverage is 77.69%.

@@             Coverage Diff              @@
##               main    #2723      +/-   ##
============================================
+ Coverage     86.05%   86.06%   +0.01%     
- Complexity     2790     2815      +25     
============================================
  Files           317      320       +3     
  Lines          8395     8527     +132     
  Branches       1048     1052       +4     
============================================
+ Hits           7224     7339     +115     
- Misses          896      912      +16     
- Partials        275      276       +1     

Impacted Files	Coverage Δ
.../mockito/internal/debugging/Java8LocationImpl.java	80.00% <ø> (ø)
...kito/internal/debugging/Java9PlusLocationImpl.java	68.04% <68.04%> (ø)
...ckito/exceptions/stacktrace/StackTraceCleaner.java	100.00% <100.00%> (ø)
...a/org/mockito/internal/MockedConstructionImpl.java	72.22% <100.00%> (ø)
...in/java/org/mockito/internal/MockedStaticImpl.java	80.51% <100.00%> (ø)
.../internal/creation/bytebuddy/MockMethodAdvice.java	80.51% <100.00%> (+0.11%)	⬆️
...rnal/creation/bytebuddy/MockMethodInterceptor.java	85.18% <100.00%> (ø)
...ockito/internal/creation/proxy/ProxyMockMaker.java	78.57% <100.00%> (+0.31%)	⬆️
...java/org/mockito/internal/debugging/Localized.java	100.00% <100.00%> (ø)
...rg/mockito/internal/debugging/LocationFactory.java	100.00% <100.00%> (ø)
... and 15 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[j-baker]

Sigh, you were right. My bad. Anyway, it looks like it'll go green now.

[TimvdLippe]

Last nits and then this is ready-to-go 🎉

[TimvdLippe]

Thanks for the hard work and investigation into performance! Much appreciated 😄

[GThinesh]

Is there any specific reason to remove the system variable to disable location tracking altogether? My teams is still using Java 8 and this is a major problem for us as well. Locally testing without location reduces the test time from 10+ min to 5 min. (Yes we have a huge code base with large entity graphs :) )

[TimvdLippe]

I prefer to not ship a flag just for Java 8, which is EOL. Instead, upgrading to Java 9 and above should yield similar benefits. Shipping flags is expensive in the longterm and in this case, I don't think it is worth carrying that cost.

[j-baker]

It’s probably worth saying: Before my changes, a simple mock call takes 3us. After, it takes 2us. If you remove all call site recording, it takes 200ns. In my case I was considering making it so that it stopped recording call sites on hotter paths (E.g. after the thousandth call to a mock or whatever) but it wasn’t a big enough blocker for me.

…

On Tue, 11 Oct 2022 at 08:53, Tim van der Lippe ***@***.***> wrote: I prefer to not ship a flag just for Java 8, which is EOL. Instead, upgrading to Java 9 and above should yield similar benefits. Shipping flags is expensive in the longterm and in this case, I don't think it is worth carrying that cost. — Reply to this email directly, view it on GitHub <#2723 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMJMN3HJWG3NJPATVORTWCUMIBANCNFSM55V5N2YQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[GThinesh]

@TimvdLippe That is true. Unfortunately we are still stuck with Java 8.

[GThinesh]

@j-baker The solution you refer the flag a counter that stops recording after certain number of invocations?

[j-baker]

I was considering proposing making the location tracking code always stop recording after enough invocations, the logic being that if you’ve done thousands of them, knowing the precise location where a single call happens from is probably less interesting. There’s possibly also something where if mock invocation recording is disabled, it might be reasonable to not record stub invocation locations.

…

On Sun, 16 Oct 2022 at 07:03, Thinesh Ganeshalingam < ***@***.***> wrote: @j-baker <https://github.com/j-baker> The solution you refer the flag a counter that stops recording after certain invocations? — Reply to this email directly, view it on GitHub <#2723 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMJP4WVCZRPOMCOVB6FTWDOLBNANCNFSM55V5N2YQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>