Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

Benchmarking/sample case #59

Closed
wants to merge 3 commits into from

Conversation

zwaldowski
Copy link
Contributor

Adds a stress test/performance benchmark to the unit tests. This is a PR to the swift-2_0 branch. Addresses #13.

@zwaldowski zwaldowski added this to the Next milestone Oct 4, 2015
@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch from cc1ad49 to 2bb44ac Compare October 7, 2015 05:53
@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch from 2bb44ac to 4e5d29f Compare October 8, 2015 18:17
@hitsvilleusa
Copy link
Contributor

I haven't looked at the details of your benchmarks, nor would I likely understand everything that you did. I imagine it's amazing, so good job!

My general thoughts are as follows:

I think that the effort you went to to compare benchmarks is a really great thing. I think that understanding the performance metrics can be a huge selling point, especially if this one is faster!

My first preference would be to have another repository which was a benchmark repo, that could perform benchmarks against different open source parsers. The work that you have done here could migrate over to that location and lay the ground work for the repo.

An alternative would be to keep this in a branch in this project, but never merge the branch into master. I am not as fond of this option, but it would at least only pollute things if you were deliberate about it.

Another, even less attractive option, is to include this in master but in a completely separate Xcode project as not to pollute the main project.

Thoughts?

@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch from 4e5d29f to f2efe53 Compare October 30, 2015 02:37
@zwaldowski
Copy link
Contributor Author

Re: this comment, lots to think about.

While initially attractive, I think maintaining a separate benchmark repo (particularly as a representative of BNR) would attract more unwanted attention than having it in this one.

I might be able to get behind keeping a separate branch, although it is fraught with maintainability issues and is somewhat against the point of the test. I want the benchmark to exist so that we strongly discourage — or even prevent — performance regressions. Ideally, if it had any notion of hardware repeatability on CI we'd get Travis to record baselines for all the performance tests and let XCTest fail branches that regress.

@zwaldowski
Copy link
Contributor Author

Updated to resolve PR dependencies.

@zwaldowski zwaldowski modified the milestones: Next, 2.0 Oct 30, 2015
@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch 6 times, most recently from 656204a to b0fdcca Compare December 9, 2015 00:09
@zwaldowski
Copy link
Contributor Author

Taking @hitsvilleusa's and @mdmathias' comments to heart, I have removed the third-party benchmark. I've also retroactively removed references to it in this PR so as to not cause any problems when we go open source. Cheers.

@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch from b0fdcca to 3fceadc Compare December 9, 2015 00:12
@zwaldowski
Copy link
Contributor Author

Again pondering @hitsvilleusa's comment, I do think a great ultimate goal would be to have a benchmarks repo of our own, but that's out of scope for this PR. I'd like to get this in so we can get #13 in this release.

@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/new-benchmarking branch from 3fceadc to eb2dab1 Compare December 9, 2015 01:11
download: Benchmark/AllSetsArray.json

Benchmark/AllSetsArray.json:
curl -L http://mtgjson.com/json/AllSetsArray.json -o Benchmark/AllSetsArray.json
Copy link
Member

Choose a reason for hiding this comment

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

I'm entertained by the data source. 😸

Copy link
Member

Choose a reason for hiding this comment

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

One issue, though: I'm not sure how well this benchmark will age without a static snapshot of the data. They don't seem to provide permalinks to a specific version, and they do seem to change the schema with some updates. Can we stash the file somewhere we'll reliably be able to grab the same version from, and toss in a sha256 of the data to verify we're getting what we think we're getting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nerds know how to make their nerdy things even nerdier, I suppose. 📦

I purposefully wanted a downloadable thing so we didn't have to check it into Git, but couldn't find something nearly as big as this at a stable URL. But, now that I think about it, the tests could break if the data set evolves (as you mention), which sucks. So… yeah.

clean:
-rm -f Benchmark/AllSetsArray.json

install:
Copy link
Member

Choose a reason for hiding this comment

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

Empty install target could be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:make:--

@jeremy-w
Copy link
Member

jeremy-w commented Dec 9, 2015

Looks good aside from the reproducibility / dataset evolution problem. If we can point curl at a static snapshot of the data, that will solve it. (I keep hoping I overlooked a permalink to a specific version of the dataset.)

@zwaldowski
Copy link
Contributor Author

14 minute build time. 😖 I'd say we could reduce the size of the payload, but considering pure Cocoa only averages 0.5s and ours 5s, I consider it a great litmus test.

Maybe if we're able to cut our parser down to, say, half that, or get even more competitive, I'd reconsider merging. But until then we should leave it on a branch, I think. Someone else respond if you have an opinion to the contrary…

@zwaldowski zwaldowski closed this Dec 17, 2015
@zwaldowski zwaldowski deleted the zwaldowski/swift-2_0/new-benchmarking branch January 2, 2016 21:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants