Lazily stringify expressions

[refi64]

Closes #556. With a trimmed down version of the example @SCross99 gave (with fewer vector elements), the time to run the test case went down by half!

It works by using type erasure to save the original given value, only converting it to a string if the test actually fails.

I tried to honor your coding convention as much as possible, so forgive me if I missed something...

[refi64]

The tests pass! 🎆

[RossBencina]

Doesn't this need a virtual dtor?

virtual ~AnyTypeHolderBase() {}

[RossBencina]

Some general observations on this patch:

The performance gain comes from a combination of two things:

1. Making expression-to-string conversion completely lazy (previously, the values were eagerly converted to strings).
2. Only calling reconstructExpression() when a check fails ((data.resultType == ResultWas::ExpressionFailed) in ResultBuilder::build().

About half the performance improvement can be had by only doing (2). (1) serves to move all of the expensive toString() conversion into reconstructExpression().

N.B. I suspect that in some cases Catch always needs the expression strings (e.g. when using --success).

[lightmare]

While this may help a bit, it doesn't avoid a significant part of stringification cost -- dynamic allocations -- you need to allocate AnyTypeHolders instead of strings.

There are ways to avoid dynamic allocations with some template tricks.

edit: deleted part that didn't make any sense, sorry for the confusion.

[RossBencina]

@lightmare this PR (#562) reduces the required allocations to 2 (1 for LHS, 1 for RHS), this is a massive improvement over master. Master incurs a lot of non-essential allocation overhead from string concatenation due to using binary operator+, and std::ostringstream (not to mention the cost of using ostringstream.) But see also my comment here: #556 (comment)

@philsquared: what are your thoughts on this type-erasure patch? Are you prepared to entertain moving away from string-based expression capture?

[philsquared]

Sorry it's been a while for me to jump in here. I've had half an eye on the discussion but not had a chance to look closely or respond.
I think the approaches discussed here are very interesting - and I'll certainly seriously consider them. However I have a feeling they may be a case of attacking the symptoms rather than the underlying problem. That's not anyone here's fault - it's simply down to the early design decisions I made which I may need to revisit.

Originally Catch was focused on pure unit testing and, as such, I felt that allocation overhead - and performance in general - was not an issue for Catch as the intended use case was not performance sensitive. So I concentrated more on keeping things simple where possible (especially important given the extra complexity I was letting in elsewhere with the expression templates).

Since then Catch has evolved to cover what I call Integration Testing (you may classify it differently: but basically any type of test that fits well with the implementation level testing of a test framework like Catch, but which doesn't meet the Michael Feathers definition of a "Unit Test"). Integration Testing can often involve volumes or complexities that do, indeed, start to run up against performance bottlenecks - so for a while I've realised that those design decisions are no longer valid. That said I still want to err on the side of simplicity over micro-optimisations.

In short I need to take a more holistic look at how things are hanging together to see if I can remove the need for allocations (for example) at all on the hotter paths. I haven't done the analysis myself, but from what I'm hearing (from you guys and others) those string conversions are probably the biggest bottleneck at the moment - but addressing those alone may not be enough.

For even more context one of the big things I'm working on for Catch 2.0 is Property Based Testing - and my early prototypes are definitely running into performance issues - so I need to address it as part of that at the very least.

So thanks for all the discussion - and PRs - around this. Please be assured I'm taking it all onboard - even if I'm not as responsive here as I'd like to be!

[horenmar]

I have different opinion actually, in that since Catch 2 is not supposed to support older compilers, performance improvements to Catch 1 are valuable, even if better architecture from the ground up could bring even larger improvements.

[lightmare]

@horenmar I've had a different patch addressing this issue in the stash for a couple months. I'll see if I can rebase on current master.

This PR is not viable, IMO. It trades premature stringification of the value for dynamically allocated copy of the value, which may be fine for simple types, but not so good for larger types, and it won't compile with noncopyable types.

[RossBencina]

I think we should aim to optimize Catch 1 as much as possible. On the other hand I agree with @lightmare that the type-erasure approach is not viable as it changes the semantics.

I have a bunch of performance improving patches that I can submit. @horenmar would you prefer one big PR, or small, clearly isolated ones that do one thing?

[horenmar]

@RossBencina I would prefer to get benchmarking suite first. If you already have one (You tested the performance improvements, right? 😄 ), that would be a nice start.

Afterwards I would strongly prefer isolated PRs where possible.