[SR-3701] [XCTest] Reduce bridging overhead from the exception-catching trampoline #6991
Conversation
By returning `nil` instead of an empty dictionary in the common case where no exception is encountered, we skip any dictionary-bridging work which can become expensive if making assertions in a tight loop.
|
@swift-ci please test |
|
@modocache may also be interested! |
| } | ||
|
|
||
| return [result retain]; | ||
| return result; |
There was a problem hiding this comment.
I wonder why the original code had retain here? I don't know what the requirements on this code are as far as the expected retain convention, but I think this is a behavior change?
There was a problem hiding this comment.
Oops, no, that was a bug. It was necessary when we were using @_silgen_name to get at the function, but it's just been leaking since I took that out. :-(
There was a problem hiding this comment.
Yeah, that's what I thought based on the comment that was also removed here
|
Out of curiosity (and apologies if this is obvious), is it dictionary bridging specifically that's expensive? If, instead of an |
|
Looks good. We might consider adding perf tests to 1) measure how big of a difference this made and 2) catch any potential future regressions right away. |
|
Bridging is expensive; returning an NSObject would avoid bridging completely, at the cost of having to downcast later on. I suggested this as an option to @ddunbar originally but I like the compromise he and @briancroom came up with if it works; it's sacrificing less type information and actually feels better for what it's trying to do. An empty dictionary and a nil dictionary really could be different here, though we put the |
|
@czechboy0 I was working with an ad-hoc performance test I whipped up while exploring this change, but I wasn't really sure how to integrate it into a suite. @jrose-apple do you think it would make sense for me to look into adding a performance test for this to the |
|
On the subject of a benchmarking suite: what was @ddunbar using when he found the regression in the first place? If that's run continuously, then it could suffice for now. |
|
@modocache I was running one of my own performance tests on a private project, and it happened to exercise XCTAssertEqual enough that this regressed its performance in a way I noticed. I probably would notice again if it happened, but its not directly tied to Swift in any way. |
|
@ddunbar Is it possible to reduce it to a test case? |
|
If it can't be, then +1 for some sort of performance test. I think the cause of the regression is too subtle for most contributors to notice, were they to undo this improvement. |
|
Issue: rdar://problem/29644454 |
|
I'll have a look at putting together a proper performance test for this. |
6 participants
I've done some experiments and am pretty confident performance regression described in SR-3701 can be attributed almost entirely to the cost of
NSDictionary->Dictionarybridging for the value returned from_XCTRunThrowableBlockBridge. By returningnilinstead of an empty dictionary in the common case where no exception is encountered, we skip any dictionary-bridging work which can become expensive if making assertions in a tight loop.Resolves SR-3701.
Alternatives Considered
We had removing the exception-catching behavior altogether, because, according to @jrose-apple, Swift does not produce exception-safe ARC code, so passing ObjC exceptions across a Swift language boundary isn't a safe operation. Additionally, support for catching ObjC exceptions thrown in an assertion expression was added prior to the advent or Swift's error handling system, and the decision hasn't been revisited since.
I am still open to the possibility of making this larger change, but I wanted to start by addressing the immediate regression.