Re: [mockito] High performance mocks

[Szczepan Faber]

Posting to the mockito-dev list (remember to reply-all). See my comments below:

On Sat, Jul 28, 2012 at 8:04 PM, H Mlnarik <hmln...@post.cz> wrote

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()?

//in MockHandlerImpl instead:

VerificationData data = new VerificationDataImpl(invocationContainerImpl, invocationMatcher);

//something like (feel free to adjust if needed):

VerificationData data = verificationDataFactory.validatedData(mockSettings, invocationContainerImpl, invocationMatcher)

 

-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?

Hmmm, I'm not seeing yet why would that be useful. I wouldn't care about captured arguments for the stubOnly() mocks.

I'd prefer something simple for now and don't worry about complex logic for the RegisteredInvocations. I.e. I would drive this change by the requirement of the feature we're developing. Anyhow - you can always prototype this in a separate branch / commit and we'll see :)

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

--
Szczepan Faber
Principal engineer@gradleware
Lead@mockito

[Hynek Mlnarik]

Hi Szczepan/Brice,

 

I have committed the changes into http://code.google.com/r/hmlnarik-mockito-stub-only/source/detail?r=8139e40170911aeac8c567cf421256c765713c05, can you please review it.

 

First, regarding your comment about capturing argument – not storing argument values is the reason why stubOnly is so much needed and has enormous performance impact. Arguments eat a lot of heap space which in turn requests much more work from garbage collector (see original thread for proof that this can easily get GC to eat 99% of time). Therefore capturing parameters must be disabled for stubs. As capturing parameters does not make much sense without capturing invocations, there is single common point where the capture/not capture should be controlled. This is currently in RegisteredInvocations. I would rather see this to be refactored a bit more so that invocations and the argument values are kept in one object to allow for e.g. selective remembering of invocations and their parameters, but this would require a bigger refactor that the one needed for the stubOnly() itself.

 

I have changed the MockCreationSettings so that it does not publish isStubOnly() but instead publishes relevant factories for creation of new VerificationData and RegisteredInvocations instances, as well as flag to capture stack trace (this is not new, all of these were in original code, hidden in conditions based on MockCreationSettings.isStubOnly(). Now isStubOnly() has been removed.).

 

The classes InvocationContainerImpl and MockHandlerImpl have been reverted to their original state, only few lines are changed to pass parameters so that collaborating classes can work with stubOnly() feature.

 

Hope we converge to the final version soon J

 

Cheers,

 

--Hynek

[Hynek Mlnarik]

Hi Szczepan/Brice,

did you have any chance to have a look at the patch?

Best,

--Hynek

On 29 čnc, 09:51, "Hynek Mlnarik" <hmlna...@post.cz> wrote:
> Hi Szczepan/Brice,
>

> I have committed the changes intohttp://code.google.com/r/hmlnarik-mockito-stub-only/source/detail?r=8...

> 70911aeac8c567cf421256c765713c05, can you please review it.
>

> First, regarding your comment about capturing argument - not storing

> argument values is the reason why stubOnly is so much needed and has
> enormous performance impact. Arguments eat a lot of heap space which in turn
> requests much more work from garbage collector (see original thread for
> proof that this can easily get GC to eat 99% of time). Therefore capturing
> parameters must be disabled for stubs. As capturing parameters does not make
> much sense without capturing invocations, there is single common point where
> the capture/not capture should be controlled. This is currently in
> RegisteredInvocations. I would rather see this to be refactored a bit more
> so that invocations and the argument values are kept in one object to allow
> for e.g. selective remembering of invocations and their parameters, but this
> would require a bigger refactor that the one needed for the stubOnly()
> itself.
>
> I have changed the MockCreationSettings so that it does not publish
> isStubOnly() but instead publishes relevant factories for creation of new
> VerificationData and RegisteredInvocations instances, as well as flag to
> capture stack trace (this is not new, all of these were in original code,
> hidden in conditions based on MockCreationSettings.isStubOnly(). Now
> isStubOnly() has been removed.).
>
> The classes InvocationContainerImpl and MockHandlerImpl have been reverted
> to their original state, only few lines are changed to pass parameters so
> that collaborating classes can work with stubOnly() feature.
>
> Hope we converge to the final version soon J
>
> Cheers,
>
> --Hynek
>

> From: Szczepan Faber [mailto:szcze...@gmail.com]
> Sent: Saturday, July 28, 2012 8:40 PM
> To: mocki...@googlegroups.com
> Cc: hmlna...@post.cz
> Subject: Re: [mockito] High performance mocks
>
> Posting to the mockito-dev list (remember to reply-all). See my comments
> below:
>

> On Sat, Jul 28, 2012 at 8:04 PM, H Mlnarik <hmlna...@post.cz> wrote

> To view this discussion on the web visithttps://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

> <mailto:mockito%2Bunsu...@googlegroups.com> .
> For more options, visit this group athttp://groups.google.com/group/mockito?hl=en.

[Szczepan Faber]

Hey Hynek,

I'm really sorry - I haven't had time for Mockito in recent weeks. I'll try to review it on Wed.

Thanks for patience :)

Cheers!

You received this message because you are subscribed to the Google Groups "mockito-dev" group.
To post to this group, send email to mocki...@googlegroups.com.
To unsubscribe from this group, send email to mockito-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito-dev?hl=en.

[Szczepan Faber]

Hey Hynek,

I did a review - sorry it took me so long :) Here's the feedback:

1. It's kind of hard to review because you've merged the revert of the
previous commit with new things. It's much easier to review if commits
are small and coherent. If you're keen on getting your work into, I'd
love to see your changes applied onto a fresh clone (or whatever works
to get a neat set of commits that can merged). What you've submitted
is pretty cool - I just don't have enough time to filter out the
unnecessary stuff from the commits.

2. Thanks for creating the RegisteredInvocations, that's exactly were
we want to go. Some feedback about this new type: in the longer run it
should become public type, a part of the API. The static concrete
class (RemoveToString) does not belong there.

3. I'm not convinced we need the 'shouldCaptureArgumentsFor' method.
This method is *only* useful (and possibly memory consuming) if
someone is using argument captor with the stubs (take a look at the
CapturingArgumentsTest). I don't believe it has impact on performance
so I don't think it makes sense to complicate the design. However, I
might be wrong - you have profiled mockito recently, so you know
better :)

4. CREATOR - why do you need the Creator instances vs just creating an instance?

5. I'd rather not add new methods to the public type
MockCreationSettings because they leak the internal types
(VerificationData, and others). You don't have to go that far with the
refactoring. I'm ok with just MockCreationSettings.isStubOnly() method
getting public, for now. After few iterations we might be ready to
expose the other methods but there's no rush to it. I didn't think you
would pick up externalizing some extra stuff to MockCreationSettings
in one go. I think it makes sense to iterate slowly (separate commits,
etc.) towards making the MockCreationSettings / stubOnly more
flexible.

Let us know if above works for you and you're still keen on helping
out. Sorry it's not an easy ride but that's how it works - you're
touching the core of Mockito ;)

Cheers!

[Hynek Mlnařík]

Hi Szczepan,

Thanks for your comments. I'll get back to you (and Mockito :-) ) hopefullly next week.

Cheers,

--Hynek

> ------------ Původní zpráva ------------
> Od: Szczepan Faber <szcz...@gmail.com>
> Předmět: Re: [mockito-dev] Re: High performance mocks
> Datum: 27.8.2012 00:09:48
> ----------------------------------------

[Hynek Mlnarik]

Hi Szczepan,

 

Thanks for the review. Please find my reactions to your individual comments here:

 

1. I have created a new clone http://code.google.com/r/hmlnarik-mockito-stubonly-feat/ where I have committed the minimal changeset for the stubOnly feature.

 

2. I moved the RemoveToString definition to the classes RegisteredInvocationsAll and RegisteredInvocationsStubOnly. I am not particularly keen on defining the same functionality twice but for the time being, it is easiest solution. Should this class be moved to outer level, please let me know the location.

 

3. We do need a mechanism to prevent capturing the argument values for stubs – as verification does not make sense for stubs, it makes no sense to verify arguments, hence capturing arguments should be blocked. In the current code, this is achieved via explicit checks to isStubOnly(), see rev. 30707da94f9c. In the previous branch, this was checked via shouldCaptureArgumentsFor().

 

4. CREATOR: this is to prevent if-else programming using standard Factory pattern. Instead of using

if (mockSettings.isStubOnly()) { return new RegisteredInvocationsStubOnly(); } else { return new RegisteredInvocationsAll(); }

    simple

return mockSettings.registeredInvocationsCreator.newInstace();

    would be used. I have used the if-then-else structure in the current implementation (see InvocationContainerImpl.createRegisteredInvocations()), yet I prefer the Factory pattern for code cleanliness and easy extensibility. The factory would allow for more configurability in MockSettings so if putting RegisteredInvocations into public API is planned, I would vote for implementing it via the factory as it was in the original code.

 

Please could you review the code? I have restricted the changes to the very minimal this time which means that I’m not completely happy with the code – still the functionality is there. Let’s discuss optimizations later, after finishing this piece. J

 

Cheers,

 

--Hynek

[Szczepan Faber]

> Thanks for the review. Please find my reactions to your individual comments
> here:
> 1. I have created a new clone
> http://code.google.com/r/hmlnarik-mockito-stubonly-feat/ where I have
> committed the minimal changeset for the stubOnly feature.

The pull request is very nice. I really appreciate the cleanness and
separate commits.

> 2. I moved the RemoveToString definition to the classes
> RegisteredInvocationsAll and RegisteredInvocationsStubOnly. I am not
> particularly keen on defining the same functionality twice but for the time
> being, it is easiest solution. Should this class be moved to outer level,
> please let me know the location.

I don't think it is big deal so long we have it tested.

> 3. We do need a mechanism to prevent capturing the argument values for stubs
> – as verification does not make sense for stubs, it makes no sense to verify
> arguments, hence capturing arguments should be blocked. In the current code,
> this is achieved via explicit checks to isStubOnly(), see rev. 30707da94f9c.
> In the previous branch, this was checked via shouldCaptureArgumentsFor().

At the moment, capturing arguments is possible in stubbing (for good
or bad). Your proposed change makes it 'silently' not working for
stubOnly() mocks. E.g. the user does not receive any error message,
it's just capturer does not capture anything for stubOnly() mocks.

Can you explain why you think we need a mechanism to make the capturer
not working for stubOnly() mocks?

> 4. CREATOR: this is to prevent if-else programming using standard Factory
> pattern. Instead of using
>
> if (mockSettings.isStubOnly()) { return new RegisteredInvocationsStubOnly();
> } else { return new RegisteredInvocationsAll(); }
>
> simple
>
> return mockSettings.registeredInvocationsCreator.newInstace();
>
> would be used. I have used the if-then-else structure in the current
> implementation (see InvocationContainerImpl.createRegisteredInvocations()),
> yet I prefer the Factory pattern for code cleanliness and easy
> extensibility. The factory would allow for more configurability in
> MockSettings so if putting RegisteredInvocations into public API is planned,
> I would vote for implementing it via the factory as it was in the original
> code.

I like the idea of hiding the complexities of creating objects into
factories, builders, etc. Yet, I don't want every object to have
unconventional means of construction. So I'd like to use Factories
when it makes sense. For example, I don't see a point in using CREATOR
for the RegisteredInvocations (your last commit). We could simply an
instance of RegisteredInvocations in mockSettings.

> Please could you review the code? I have restricted the changes to the very
> minimal this time which means that I’m not completely happy with the code –
> still the functionality is there. Let’s discuss optimizations later, after
> finishing this piece. J

I've reviewed it I'm trying to figure out how to merge it now. It's
not easy as mercurial and google code are not super easy to do this
thing. I cannot merge all stuff as I don't like some things in the
last commit and I'd like to iterate on that a bit. Nevertheless most
of your stuff should end up merged soonish :)

I'll summarize more feedback later.

Cheers!