Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocationImpl adds performance overheads due to instantiating a stack trace #2720

Closed
j-baker opened this issue Aug 4, 2022 · 2 comments · Fixed by #2723
Closed

LocationImpl adds performance overheads due to instantiating a stack trace #2720

j-baker opened this issue Aug 4, 2022 · 2 comments · Fixed by #2723

Comments

@j-baker
Copy link
Contributor

j-baker commented Aug 4, 2022

I'm trying to use Mockito in a test of a Java class I didn't write. It takes a dependency which I am stubbing out using Mockito. Skipping all of the business logic, the code ends up being rather similar to this:

SomeClass mocked = mock(SomeClass.class, Mockito.withSettings().stubOnly());
when(mocked.someParam()).thenReturn(0);

for (int i = 0; i < bigNumber; i++) {
    mocked.someParam();
}

This is in Mockito 4.6.1. This is extremely slow, and looking at Mockito this seems to be a problem for two reasons.

Firstly, when a mock invocation occurs, LocationImpl is instantiated. This instantiates a Throwable, which constructs a stack trace, error message, etc (which is then discarded). In my test, Mockito was generating 70k exceptions per second.

The second slowdown that seemed to be affecting me comes from TypeSafeMatching.getArgumentType. This iterates through all of the methods on the argument matcher class to find the appropriate matches method, seemingly also for each method call. By performing this operation, a lot of JVM internal datastructures get copied - it calls Class.getMethods, which copies all the methods on the class. This method could probably be cached on a per-class basis.

This dramatically affected my application's allocation rate; Mockito was leading to approx 3.4GiB/sec allocations. In the end, I rewrote my mock using ByteBuddy proxies directly. It took the test runtime from 30 seconds down to 1, with the number of exceptions being created per second going down from 70k to ~0.

I know that Mockito isn't meant to be a Netty-like high performance library, but wanted to flag these two codepaths as affecting every mock call.

@TimvdLippe
Copy link
Contributor

public StackTraceElement filterFirst(Throwable target, boolean isInline) {
is supposed to make this fast if the JVM supports it, but I am not sure if that still works. #2225 was supposed to fix it, but I had to abandon it.

Maybe you can use these to fix the performance problems?

@j-baker
Copy link
Contributor Author

j-baker commented Aug 4, 2022

Yes, the method currently used relies on JavaLangAccess containing a getStackTraceElement method. The diff that integrated the StackWalker api deleted this and so this regression has existed since Java 9.

openjdk/jdk@eb2c6c5#diff-9908a97582edc388983be46caac4de91a4b958df46d86b9bd739f577a55c01b0L105

j-baker added a commit to j-baker/mockito that referenced this issue Aug 5, 2022
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 state. The StackTraceElement conversion
  will only occur (internally) if the .getFileName() method is called.
- 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.

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.

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.
j-baker added a commit to j-baker/mockito that referenced this issue Aug 5, 2022
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 state. The StackTraceElement conversion
  will only occur (internally) if the .getFileName() method is called.
- 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.

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.

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.
TimvdLippe pushed a commit that referenced this issue Aug 13, 2022
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 state. The StackTraceElement conversion
  will only occur (internally) if the .getFileName() method is called.
- 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.

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants