Excessive locking in TypeCachingBytecodeGenerator#BOOTSTRAP_LOCK #3095
Conversation
|
I know that the MockitoKey is finicky and that we previously broke the caching and nuked performance with this. @raphw Can you please look at this change and verify we don't reintroduce that behavior? I also remember us talking about writing a regression test for this and I don't think we ever did that. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3095 +/- ##
=============================================
+ Coverage 12.39% 85.45% +73.06%
- Complexity 422 2889 +2467
=============================================
Files 329 329
Lines 8813 8822 +9
Branches 1094 1095 +1
=============================================
+ Hits 1092 7539 +6447
+ Misses 7510 995 -6515
- Partials 211 288 +77
☔ View full report in Codecov by Sentry. |
|
For reference, this was the PR that reverted the bad patch: #2402 |
|
I think the issue would be if somebody created a mock for interface A with additional interface B while another thread created a mock for interface B with additional interface A, or with different settings. I mildly remember that the bug came from this corner. As far as I recall, the lock is only hold if the mock does not yet exist? We concluded that the costs of mocking are much higher than the costs of locking, so it would not be a problem. If we can improve this, I welcome it, but I think the key must be broader to cover the mentioned scenarios. |
|
Do I understand that correctly, that the required lock to hold must cover the |
|
Yes, depending on the settings Mockito needs to create multiple mock types for the same base type. |
|
@raphw I have changed the locking behavior to account for the data in the |
|
That's a good idea. We should however add a test that stresstests this, one with a purpuseful hash collision, and one without. |
Changed locking scheme in TypeCachingBytecodeGenerator from a single global lock to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of MockitoMockKey, which aggregates the types and settings to mock. Fixes mockito#3035
|
@raphw done. |
|
Looks good to me! |
|
@TimvdLippe The tests are not failing if I revert my change, because the old code was also correct, just more blocking. Flight recorder measurement.
The stacktrace of the image, is the stack of the right side. |
|
@TimvdLippe Out of cusiosity, how about I open another PR to integrate Java FlightRecorder measurments for tests if you specify the flag |
|
Yes that sounds like a great idea. If this test doesn't fail, I think it makes sense if we create a new subproject called |
|
It would fail, if the locking is somehow broken. And the test are not that slow on my machine. So I would say these still make sense to always execute them. |
|
That works for me! Looking forward to your next PR to integration the JFR monitoring as well |
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types and settings to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types and settings to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types and settings to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types and settings to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095 Co-authored-by: Björn Kautler <Bjoern@Kautler.net>
Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095 Co-authored-by: Björn Kautler <Bjoern@Kautler.net>
…1778) Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: mockito/mockito#3095
Changed locking scheme in TypeCachingBytecodeGenerator from a single global lock to cacheLocks with size 16.
The used TypeCachingLock from cacheLocks depends on the hashcode of MockitoMockKey, which aggregates the types and
settings to mock.
The old global
BOOTSTRAP_LOCKdid blockThreadsregardless, if they try to mock the same type or not.Fixes #3035
Checklist
including project members to get a better picture of the change
commit is meaningful and help the people that will explore a change in 2 years
Fixes #<issue number>in the description if relevantFixes #<issue number>if relevant