Re: [mockito] High performance mocks

[Brice Dutheil]

HI,

Yes of course, if you have some spare time to work on this feature :)

I think the best place to look up is actually the MockHandlerImpl. I always wanted to tidy up the handle method anyway, this could be the right opportunity. I'm not really sure if it will be enough atm, but you'll probably need the code for stubbing (beginning of handle) and the code to replay the stubs (end of handle) only.

Maybe Szczepan or some others can give you a deeper knowledge on this matter.

Cheers,

-- Brice

On Thu, Jul 26, 2012 at 8:30 AM, H Mlnarik <hmln...@post.cz> wrote:

Hi,

I am wondering whether I can help with work on issue 84. I would very much appreciate the stubOnly settings on the mocks. Mockito offers great way of partial implementation of interfaces for test purposes. Not having stubOnly option however disqualifies it from using these mocks: when there are thousands of invocations of mocked methods, mockito quickly goes out of memory.

I can help in implementing this feature if anyone points me to the right place. I might be overlooking some things, but is it harder than adding MockSettings.stubOnly() and checking the value of it before captureArgumentsFrom() is called in MockHandler?

Best,

--Hynek

--
You received this message because you are subscribed to the Google Groups "mockito" group.
To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/GYhzH281DgoJ.
To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

[H Mlnarik]

Hi,

I have prepared a changeset for that change, it is available at http://code.google.com/r/hmlnarik-stub-only/source/detail?r=d4083a979f28ecb52950b5103c7bc1c2b9f67e32. Could anyone please review it?

Thanks,

--Hynek 

[Brice Dutheil]

Hi,

Wow you are fast :)

I'll try to find some time to review it, probably not today anyway :/

And this week-end I might be really busy.

I think this performance issue raises needs in profiling or at least benchmarking. Thinking loudly it might be interesting to create such a project with Caliper for example.

-- Brice

--

You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/sEqCm0aZ844J.

[Brice Dutheil]

Hi,

Regardless of the following remarks : your changeset is pretty good !

So the few remarks still :)

a. Your concurrency changes should have been in separate changeset, as it doesn't seem related to 'stubOnly'. So you could give a better description in the commit message why it was changed :)

Oddly enough, this is the first time I got this test finally failing : ConcurrentModificationExceptionOnMultiThreadedVerificationTest.

It still is random though, the only opportunies for this bug to arise on my current machine were on a fresh start of IntelliJ and fresh compilation from the last session, and when I run 'All tests in Mockito'.

I'm not sure wether your code expose expose this behavior or not, if you have a better input on it will be appreciated.

b. It's not in any FAQ yet, it's in some thread of the dev mailing-list; but it might interest you for future work if you wish so :)

So we decided to use underscored naming for test methods, and we decided if a test is changed then we will update the tests method name accordingly in this class.

c. For Reporter we don't use static methods.

Oh by the way did you had the opportunity to *measure* the benefits of this option ?

-- Brice

[H Mlnarik]

Hi Brice,

thanks for your review. :-) I have started a new branch and committed the changes in separate chunks as you suggested. They start in http://code.google.com/r/hmlnarik-mockito-stub-only/source/detail?r=96687d1c08cf712a46aa91ddcf1a29f0fb30d987. Can you please review the code? I have addressed all your comments although I am not very keen on creating a new Reporter instance when a static method would be enough - this leads to an unnecessary (young) object to be garbage collected. Still, if you chose that, I respect it.

Regarding the names of test methods, I think it can be easily automated by running the following command if you like to (indeed, manual check needs to be done whether it did not change more than expected):

find ./test -name '*.java' | xargs perl -i -pe 's/(?<=[a-z])([A-Z])/_\l$1/g if (/void \S+\(\)/)'

Regarding the performance tests, I did a microbenchmark (source code is attached; no library used, just System.nanoTime()) and found that:

• A stub-only mocks perform roughly at 30% of time of full-featured mocks
• Most importantly, stub-only mocks do not request garbage collector to perform any harder than usually and keep heap free of garbage. This is on the contrary to full-featured mocks that can easily get garbage collector to swallow 99% of time (thus making stub-only mocks relatively even faster :-) ). But seriously - see attached pictures of VM telemetry. They were taken when the test from the attachment was run. There are three important points marked - first is the start, when the test was warmed up and stub-only test is about to start. Second, when the stub-only test finishes and full-featured mocks are tested. Third - when this test finishes. The test results clearly provide arguments supporting my previous sentence. It took me a little while to find such a combination of parameters so that the test finished before reaching 99% of time spent in GC. This time is due to quick creation of tons of live objects and filling the heap, see the second picture.

GC:

Heap:

I believe that the stubOnly is hence a quite useful feature that should find its way to trunk. Once again, thanks for your efforts in reviewing my bits and also for your work for Mockito! 

Cheers,

--Hynek

[Brice Dutheil]

Wow that's awesome work !

You really should take a look at Caliper as well, though I don't think it will be possible to monitor easily the heap.

About Reporter, as it's made to report error, I don't think that conditional allocation that will really cripple the GC (in this erroneous path).

I don't know if it will go in the next release as it's somehow a new feature, we'll have to discuss that in the team. However it will land in the default branch (...or master if we speak git).

Regards,

-- Brice

--
You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/EgjAyPOsQvcJ.

[H Mlnarik]

Hi Brice,

I did a bit more profiling with the test and found that most of the time in handling methods is spent in CGLIBHacker.setMockitoNamingPolicy() method (see picture below). When I effectively disabled the method, the tests ran much faster:

• For full-featured mocks, minimum time needed to finish the tests dropped up to 39% of the original run time, saving 61%
• For stubs, time dropped up to 26% of original time, saving 74%

This method is only used to add tag "ByMockitoWithCGLIB" to generated class names. Is it worth it? Shall I add an option to disable it? Or should this be considered for complete removal?

Best,

--Hynek

[Szczepan Faber]

Hey,

Regarding the CGLIBHacker - it's needed because by default certain cglib-internal generated classes do not use the Enhancer's naming policy when they are created. They are created when Objenesis instantiates the mock. So we hack in the naming policy via reflection which apparently is costly :) The naming policy needs to be there otherwise Mockito mocks clash with other frameworks (the bug in question was raised with the mockito+hibernate combo AFAIR).

To fix the problem we could change the the cglib code to allow configuring the naming policy via the getter on the MethodProxy instance (or something like that). We already package the cglib-and-asm for other reasons. This way we avoid using reflection and performance is happy.

Very nice catch, thanks for finding this bottleneck.

Cheers!

Best,

--Hynek

--

You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/iV8Zv5rH8n8J.

To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

--
Szczepan Faber
Principal engineer@gradleware
Lead@mockito

[Szczepan Faber]

Not bad, thanks a lot for contributing :)

I've scanned the code, some feedback (sorry I'm late).

1. Very cool you got that working - nice dive in the Mockito codebase. Good thinking about the tests and validation!

2. I would avoid making so much changes in the MockHandlerImpl. Perhaps your refactorings are making it better but I just won't have time review them soon :/ We should also try to reduce the complexity of MockHandlerImpl, that is try to not put extra stuff there.

3. Due to #2, here's my suggestion on how to go about coding it:

-pass mock settings to InvocationContainerImpl so that it can create simpler / no-op version of  RegisteredInvocations for stubs.

-add a new object that validates the verification. This will be used in MockHandlerImpl. It should also include the toString() validation we already have in VerificationDataImpl, thus making VerificationDataImpl simpler.

-Not required to implement this feature but might be useful if we want to keep the bar high :P. We could create a public interface for RegisteredInvocations and make them travel with the mock settings. This way users could pass custom implementations of RegisteredInvocations via settings. The stubOnly() feature would use exactly that. We also would have to expose the verification validation somehow.

Hope that helps!

Cheers!

--
You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/EgjAyPOsQvcJ.

To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

[H Mlnarik]

Hi Szczepan,

Thanks for the feedback. Just before I would start making some changes, question about #2 - I have refactored the MockHandlerImpl.handle() on suggestion by Brice (to "tidy up the handle method"). Should I revert the changes then to be as close as possible to the original version?

Best,

--Hynek

[Szczepan Faber]

Thanks for the feedback. Just before I would start making some changes, question about #2 - I have refactored the MockHandlerImpl.handle() on suggestion by Brice (to "tidy up the handle method"). Should I revert the changes then to be as close as possible to the original version?

Yes please ;) It would be cool to keep the complexity outside the MockHandlerImpl. 

Cheers!

 

Best,

--Hynek

On Saturday, July 28, 2012 5:40:01 PM UTC+2, szczepiq wrote:

Not bad, thanks a lot for contributing :)

I've scanned the code, some feedback (sorry I'm late).

1. Very cool you got that working - nice dive in the Mockito codebase. Good thinking about the tests and validation!

2. I would avoid making so much changes in the MockHandlerImpl. Perhaps your refactorings are making it better but I just won't have time review them soon :/ We should also try to reduce the complexity of MockHandlerImpl, that is try to not put extra stuff there.

3. Due to #2, here's my suggestion on how to go about coding it:

-pass mock settings to InvocationContainerImpl so that it can create simpler / no-op version of  RegisteredInvocations for stubs.

-add a new object that validates the verification. This will be used in MockHandlerImpl. It should also include the toString() validation we already have in VerificationDataImpl, thus making VerificationDataImpl simpler.

-Not required to implement this feature but might be useful if we want to keep the bar high :P. We could create a public interface for RegisteredInvocations and make them travel with the mock settings. This way users could pass custom implementations of RegisteredInvocations via settings. The stubOnly() feature would use exactly that. We also would have to expose the verification validation somehow.

Hope that helps!

Cheers!

--
You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/PjhuXdKR6twJ.

To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

[Brice Dutheil]

3. Due to #2, here's my suggestion on how to go about coding it:
-pass mock settings to InvocationContainerImpl so that it can create simpler / no-op version of  RegisteredInvocations for stubs.
-add a new object that validates the verification. This will be used in MockHandlerImpl. It should also include the toString() validation we already have in VerificationDataImpl, thus making VerificationDataImpl simpler.
-Not required to implement this feature but might be useful if we want to keep the bar high :P. We could create a public interface for RegisteredInvocations and make them travel with the mock settings. This way users could pass custom implementations of RegisteredInvocations via settings. The stubOnly() feature would use exactly that. We also would have to expose the verification validation somehow.

@Szczepan Wow you are indeed setting the bar high :)

@Hynek Sorry for doubling the work, I was thinking of a much lower profile patch. Still, I think the code extraction you performed were correct and useful (even without the stubOnly stuff).

Cheers,

-- Brice

[H Mlnarik]

3. Due to #2, here's my suggestion on how to go about coding it:

-pass mock settings to InvocationContainerImpl so that it can create simpler / no-op version of  RegisteredInvocations for stubs.

Ok, straightforward.

 

-add a new object that validates the verification. This will be used in MockHandlerImpl. It should also include the toString() validation we already have in VerificationDataImpl, thus making VerificationDataImpl simpler.

Could you please be more elaborate on that? Do you mean that MockCreationSettings would provide a factory for creation of VerificationData instances that would be creating instances of different classes depending on stubOnly()?

 

-Not required to implement this feature but might be useful if we want to keep the bar high :P. We could create a public interface for RegisteredInvocations and make them travel with the mock settings. This way users could pass custom implementations of RegisteredInvocations via settings. The stubOnly() feature would use exactly that. We also would have to expose the verification validation somehow.

This would be also straightforward but there is a problem with remembering parameters. The invocations are remembered in invocationContainerImpl.setInvocationForPotentialStubbing() while their parameters are stored in stubbedInvocation.captureArgumentsFrom(...). Each parameter retains its own list of passed values (in CapturingMatcher.arguments) which is independent of  invocationContainerImpl.registeredInvocations. Should a more complex logic for RegisteredInvocations be implemented, this should also reflect in logic of capturing parameters.

Therefore I would introducing a method captureArguments(StubbedInvocationMatcher sim, Invocation i) in RegisteredInvocations that would delegate to sim.captureArgumentsFrom(i) for mocks and do nothing for stub-only. Do you agree?

Best,

--Hynek

[Szczepan Faber]

I moved the thread to the mockito-dev list.

See you there! :)

--

You received this message because you are subscribed to the Google Groups "mockito" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mockito/-/jJOGdrDYRfAJ.

To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.