Skip to content

[SR-1355] Add Performance Measurement APIs #109

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

Merged
merged 1 commit into from
May 12, 2016

Conversation

briancroom
Copy link
Contributor

What's in this pull request

This is an initial implementation of the XCTest performance measurement APIs. All public facing APIs from Apple XCTest are present, but not all behaviors are implemented at this point. It is currently possible to measure the wall clock time that a given block of code takes to execute, and see the measured times, the mean value, and the relative standard deviation in the printed test output.

The core behaviors mimic those described in the Apple XCTest framework headers, supplemented by information presented in the Testing in Xcode 6 WWDC session.

  • The code being measured is always executed exactly 10 times (assuming no test failures occur)
  • A test failure is triggered if the relative standard deviation of the times measured is greater than 10%, and the standard deviation value is greater than 0.1

This does not include any support for recording baseline values or triggering test failures due to a regression relative to a baseline.

Resolves SR-1355.

Code structure

Although the build system does not currently facilitate the use of multiple Swift modules, I have structured the code with modularity in mind. The PerformanceMeter.swift and WallClockTimeMetric.swift contain the heart of the implementation and have (almost) no dependence on other parts of the XCTest library. XCTestCase+Performance.swift contains the public APIs which delegate to the PerformanceMeter class.

Test coverage

There are extensive functional tests covering of API misuse scenarios and basic measurement output, however many finer points (e.g. standard deviation calculation and situations triggering failure) have only cursory tests due to the lack of a usable unit testing infrastructure (see this PR for previous discussion about this.)

I attempted to make the tests robust, however due to the time-sensitive nature of the APIs under test, there is an increased possibility of flakiness that could appear when running in a CI environment. I would greatly appreciate feedback on this aspect of the test cases and what approaches we could take to minimize the risk of false negative results. I would also like to hear feedback on the test_measuresWallClockTimeInBlock test case in Performance/main.swift, which currently includes a 1-second sleep in order to
test that time passing is being measured.

Error reporting

I took pains to implement failure reporting due to API misuse, just as Apple XCTest does. One element of this is reporting the file and line number where the misuse occurred. I have taken cues from @modocache's XCTestExpectation implementation and added file and line params where necessary to capture that information. The current implementation records test failures for API misuse, allowing test suite execution to continue at the cost of some additional bookkeeping code. An alternative would be to simply fatalError in these scenarios, since they are clearly programmer error. I would appreciate others' input on these options.

Time measurement

I am no expert in benchmarking, so I am not especially confident in the optimal mechanism for measuring the passage of time for this purpose, however after some cursory research, I settled on using NSProcessInfo's systemUptime property, which ultimately calls down to mach_absolute_time on Darwin, and clock_gettime with CLOCK_MONOTONIC on Linux. As far as I can tell, these are the appropriate primitives to use for this purpose in these environments. One potential source of error is introduced in that the time values are being converted to Double, but I'm unsure of the practical impact of that.

While not complete, this is still a significant step towards feature parity with Apple XCTest, and should be sufficient to start gathering feedback on future implementation directions. In particular, it should put us in a good position to productively carry on discussions like this one from @drewcrawford which have great potential to improve the value that these tools provide.

cc @ddunbar @paulofaria

… metric

* Failures can be triggered by a sufficiently high STDDEV value on a set
  of measurements
* There is no support for saving baselines or failing due to regressions
@briancroom
Copy link
Contributor Author

@swift-ci please test

@modocache
Copy link
Contributor

modocache commented May 12, 2016

Amazing!! Thanks for this. Swift 3.0, here we come!!

Although the build system does not currently facilitate the use of multiple Swift modules, I have structured the code with modularity in mind.

Is this something you think we should address? Are there consequences for end-users if we split it up, such as requiring them to link multiple modules, besides XCTest?

XCTestCase+Performance.swift contains the public APIs which delegate to the PerformanceMeter class.

This has been in the back of my mind for a while now: Apple places its asynchronous testing APIs in a category on XCTestCase, placed in a file called XCTestCase+AsynchronousTesting.h. Do you think we should split the asynchronous testing portion of SwiftXCTest.XCTestCase into an extension in its own file? Following your naming convention, perhaps XCTestCase+Asynchronous.swift? Or is this not worth the effort? One tricky part is that XCTestCase uses an internal variable to hold onto the expectations it's vended, whereas we can't place additional variables on extensions... Oh, I see _performanceMeter is also defined on XCTestCase, even though all the logic that uses it is in an extension. So I guess there's no problem if the async logic uses the same pattern.

More granular comments to follow.

func recordMeasurements(results: String, file: StaticString, line: UInt)
func recordFailure(description: String, file: StaticString, line: UInt)
func recordAPIViolation(description: String, file: StaticString, line: UInt)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The awesome documentation for PerformanceMetric spoiled me, now I wish there was documentation here, too 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call!

@modocache
Copy link
Contributor

This looks fantastic! I only had minor nit-picks, which I feel could adequately be addressed after merging. Considering:

  1. The API matches Apple XCTest
  2. We can make improvements over time (the implementation doesn't have to be perfect right now)
  3. CI is passing

I'd encourage you to hit the big green merge button. 😄

I don't know much about time measurement, but as for the other things you mention in your pull request:

  • Code structure: 💯
  • Test coverage: I agree that unit tests would be nice, but I think we can wait for Add Unit Testing Infrastructure #61 to add those. A one second sleep doesn't seem like a big deal to me at this point in this codebase's life, but it'd be nice to make it shorter if possible.
  • Error reporting: If Apple XCTest reports failures and keeps executing, I'd prefer to do that. fatalError seems harsh -- but again, I think we should always do whatever Apple XCTest does. Asynchronous tests report failures for this reason.

@ddunbar
Copy link
Contributor

ddunbar commented May 12, 2016

The time measurement parts sound reasonable to me for an initial implementation:

  1. Imprecision w.r.t. conversion to Double is a non-issue for this level of API.
  2. Using mach_absolute_time is the right answer on OS X, via a wrapper in Foundation seems reasonable to me.

@briancroom
Copy link
Contributor Author

Thanks for the positive feedback guys! 😎 I'm going to go ahead and merge this. Exciting that it appears to be making it in before the creation of the swift-3.0-preview-1-branch.

@briancroom briancroom merged commit dbcf097 into swiftlang:master May 12, 2016
@briancroom briancroom deleted the performance branch May 12, 2016 21:30
@briancroom
Copy link
Contributor Author

Oh, regarding modules - I do believe it should be possible to split the library internals into multiple modules without require consumers to modify their imports (which would make this a non-starter), but I'm not certain yet. @ddunbar or @mxcl could likely confirm the feasibility of this given their experience with SwiftPM?

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.

3 participants