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

Improve performance of MockExpectedCallsList #778

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mmartalo
Copy link

For large number (e.g. 10000) of mocked calls, the simple linked list
was a performance bottleneck.
This patch brings two improvements:

  • tracking the last element of the linked list (for O(1) insertion)
  • deleting NULL elements as they are marked, as opposed to rescanning
    the list from scratch after marking.
    Profiling with callgrind shows that the linked list structure is no
    longer the performance bottleneck.

I know that having a huge number of mocked/expected calls is debatable, but it did happen to matter in my use case. Basically, I'm doing checks of the type: perform an operation, and assert that a certain log message is displayed. This means that MockExpectedCallsList has to scale to the number of logs printed by the operation.

As you see from the Callgrind results for this particular test case, addExpectedCall was spending most of its time in self(). Where the vast majority of this time went was into looping through the linked list in order to find the tail, so that it can insert the new element.

Profiling overview before patch:
callgrind results before patch
Profiling before patch, zoom in on addExpectedCall:
callgrind zoom in on addexpectedcall
Profiling overview after patch:
callgrind results after patch

For large number (e.g. 10000) of mocked calls, the simple linked list
was a performance bottleneck.
This patch brings two improvements:
- tracking the last element of the linked list (for O(1) insertion)
- deleting NULL elements as they are marked, as opposed to rescanning
  the list from scratch after marking.
Profiling with callgrind shows that the linked list structure is no
longer the performance bottleneck.
// of pruneEmptyNodeFromList(), and letting the "parent = p" execute normally in the for loop).
//
// You need to close the curly bracket after the processing step.
#define ITERATE_WITH_PREV_AND_NEXT \
Copy link
Author

Choose a reason for hiding this comment

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

I think keeping the iteration algorithm in one place is good, but I'm ok with inlining this everywhere if you find it ugly.

PREV should be changed to PARENT

@arstrube
Copy link
Contributor

Great!

Do you suppose you could do the same without using Std C Lib assert()? The thing is that CppUTest shouldn't depend on the Std C library, and the inclusion of assert.h breaks that.

This is why you get a failure on Travis CI - it's a run specifically designed to check for compilation without Std. C library.

Asserts left in the code as comments
@mmartalo
Copy link
Author

Oh ok, I didn't know that. I was wondering how assert.h could be missing :)
I removed the asserts. I left them as comments, I think they help reading the code, but I can remove them completely if they seem overkill.

@arstrube
Copy link
Contributor

There you go. About the comments / asserts, let's see what @basvodde has to say. Looking forward to this being merged....

@mmartalo
Copy link
Author

mmartalo commented Sep 7, 2015

Any news on this? If the PR needs changes, please let me know this week, as I'll be unavailable for a couple of weeks afterwards.

@basvodde
Copy link
Member

basvodde commented Sep 9, 2015

Sorry! I won't have time to merge it this week, but I'll definitively will request some small changes. That said, I'm pretty sure eventually it'll be merged as this was one problem that needed to be solved. So thanks for that and sorry for the slow response during these days.

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