Lazily render mock diff output on successful match #1615
Conversation
|
This includes the CallerInfo change from #1614 |
There was a problem hiding this comment.
I've considered and tried a similar approach before when trying to address #1597. It only removes the race from passing tests, though. If it comes with a sizable performance benefit I'm more than happy to lazily render these.
I'd like it to be separated from #1614, they're not dependant on one another, right?
Then there's some changes I'd like around the use of outputRenderers. I'm fairly sure the slice isn't even necessary, you can change actualFmt's type from string to func() string and change all its references to actualFmt().
assert/assertions.go
Outdated
| if !more { | ||
| // We know we already have less than a buffer's worth of frames | ||
| if !maybeMore { | ||
| break | ||
| } | ||
| offset += stackFrameBufferSize | ||
| n = runtime.Callers(offset, pcs) | ||
| if n == 0 { | ||
| break | ||
| } | ||
|
|
||
| maybeMore = n == stackFrameBufferSize | ||
|
|
||
| frames = runtime.CallersFrames(pcs[:n]) | ||
| } |
There was a problem hiding this comment.
I'm not a fan of more and maybeMore, this code is actually two nested loops and I think it would be more readable if you coded it that way. I think that pure behaviour is usually better than data (varibles) used only to alter behaviour.
This code is also from #1614, it might be quicker to review this change if you keep the two PRs separate.
There was a problem hiding this comment.
I split this back out. My suspicion is that I opened PRs both on upstream and on our fork, and in the process of merging this into master on our fork the branch got updated.
| var differences int | ||
|
|
||
| maxArgCount := len(args) | ||
| if len(objects) > maxArgCount { | ||
| maxArgCount = len(objects) | ||
| } | ||
|
|
||
| for i := 0; i < maxArgCount; i++ { | ||
| outputRenderers := []outputRenderer{} |
There was a problem hiding this comment.
interface{} says nothing, the case for this being type string isn't actually necessary, you can just use a static function. Then you no longer need the unreachable panic.
| outputRenderers := []outputRenderer{} | |
| outputRenderers := []func() string |
There was a problem hiding this comment.
Yeah I waffled back and forth on that for a bit. Can definitely simplify every renderer down to func() string
I honestly don't remember if they are or are not given how long it's been since I wrote this. I can try to take a pass on cleaning this and the other PR up after next week (short week for thanksgiving in the states) |
mikeauclair
force-pushed
the
mauclair-mock-match-sprintf
branch
from
d45244f to
3d98e69
Compare
Summary
Sprintf-ing theactualvalue can be costly. Switch successful matches to use thunks to lazily render their output, and fully avoid rendering their output when there are no differences since we return a constant string in that case anywaysChanges
Motivation
<Whatever>Requeststructs that are costly to%vviaSprintf.Sprintfshowed up as a tangible contributor to runtime performance when profiling some of our heavier tests. The below ~5s cpu time spent onSprintfgoes away completely here in one of our exemplar testsRelated issues