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

TypeSafeMatching no longer iterates over class methods inefficiently #2729

Merged
merged 1 commit into from Aug 13, 2022

Conversation

j-baker
Copy link
Contributor

@j-baker j-baker commented Aug 13, 2022

Fixes #2723.

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).

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

Fixes mockito#2723.

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).
@j-baker
Copy link
Contributor Author

j-baker commented Aug 13, 2022

I saw you triggered CI and it failed. I've edited the PR subsequently and it certainly passes locally...

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #2729 (c7312d3) into main (70c1fe9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2729      +/-   ##
============================================
+ Coverage     86.27%   86.28%   +0.01%     
  Complexity     2795     2795              
============================================
  Files           317      317              
  Lines          8395     8402       +7     
  Branches       1046     1049       +3     
============================================
+ Hits           7243     7250       +7     
  Misses          879      879              
  Partials        273      273              
Impacted Files Coverage Δ
.../mockito/internal/invocation/TypeSafeMatching.java 91.66% <100.00%> (+3.43%) ⬆️

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

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants