Skip to content

Commit

Permalink
Optimize TypeSafeMatching iteration over class methods (#2729)
Browse files Browse the repository at this point in the history
`Class.getMethods` is an inefficient method call which is being called on
each mock invocation. It ends up constructing new `Method` objects for
each method on the class, and this can dominate the overall performance
of Mockito mocks. This commit caches the result of the computation.

Once concern is that this change uses some static state. I considered:

- Instance state - based on where this type is constructed it seemed
  like it'd be a big imposition on code readability elsewhere.
- Weakly referenced map. Mockito has a type for this, but the
  constructor of that type produces a Thread with which to clean up.

Both of these seemed like overkill compared to the overhead expected
in the real world (which should be on the order of a few kilobytes of
RAM at most).

Fixes #2723
  • Loading branch information
j-baker committed Aug 13, 2022
1 parent 70c1fe9 commit 89698ba
Showing 1 changed file with 26 additions and 3 deletions.
Expand Up @@ -4,15 +4,26 @@
*/
package org.mockito.internal.invocation;

import java.lang.reflect.Method;

import org.mockito.ArgumentMatcher;

import java.lang.reflect.Method;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

@SuppressWarnings({"unchecked", "rawtypes"})
public class TypeSafeMatching implements ArgumentMatcherAction {

private static final ArgumentMatcherAction TYPE_SAFE_MATCHING_ACTION = new TypeSafeMatching();

/**
* This cache is in theory unbounded. However, its max size is bounded by the number of types of argument matchers
* that are both in the system and being used, which is expected to bound the cache's size to a low number
* (<200) in all but the most contrived cases, and form a small percentage of the overall memory usage of those
* classes.
*/
private static final ConcurrentMap<Class<?>, Class<?>> argumentTypeCache =
new ConcurrentHashMap<>();

private TypeSafeMatching() {}

public static ArgumentMatcherAction matchesTypeSafe() {
Expand All @@ -39,11 +50,23 @@ private static boolean isCompatible(ArgumentMatcher<?> argumentMatcher, Object a
return expectedArgumentType.isInstance(argument);
}

private static Class<?> getArgumentType(ArgumentMatcher<?> matcher) {
Class<?> argumentMatcherType = matcher.getClass();
Class<?> cached = argumentTypeCache.get(argumentMatcherType);
// Avoids a lambda allocation on invocations >=2 for worse perf on invocation 1.
if (cached != null) {
return cached;
} else {
return argumentTypeCache.computeIfAbsent(
argumentMatcherType, unusedKey -> getArgumentTypeUncached(matcher));
}
}

/**
* Returns the type of {@link ArgumentMatcher#matches(Object)} of the given
* {@link ArgumentMatcher} implementation.
*/
private static Class<?> getArgumentType(ArgumentMatcher<?> argumentMatcher) {
private static Class<?> getArgumentTypeUncached(ArgumentMatcher<?> argumentMatcher) {
Method[] methods = argumentMatcher.getClass().getMethods();

for (Method method : methods) {
Expand Down

0 comments on commit 89698ba

Please sign in to comment.