[emkornfield]

This isn't yet complete, but before I go further I think its worth asking some questions on peoples' preferences:

1. It seems that the build third-party script is setting up an install directory that it is not making use of. Do we want to keep this functionality and start adding new libraries to be placed there? The gtest component of the tool-chain assumes it is in its own location, and this how I patterned google benchmark integration.
2. Do we want to couple unit test builds with benchmark builds? I am currently aiming for having them decoupled and having benchmarks off by default.
3. I am not familiar with the Darwin/mac build environment and it is not clear if the CXX flags are required universally. (I need to fix it anyways to move -DGTEST_USE_OWN_TR1_TUPLE=1 back to be gtest only). Travis-ci might provide the answer.
4. Any other basic features in the benchmark toolchain people would like to see as part of this PR? Wes mentioned starting to create benchmarking tools lib, but I think that likely belongs in a separate PR.

[emkornfield]

For point item 4. I mean in addition to the obvious of adding the ability to add/automatially run benchmark tests.

[wesm]

change DIRECTORY to PATH here for cmake 2.8

[wesm]

1. In the case of gtest, it is actually not intended to be "installed" -- there is no homebrew recipe for googletest and it is not in most Linux package managers. If gbenchmark is the same, we can follow that, but otherwise we should put it in thirdparty/installed. Check out https://github.com/apache/parquet-cpp/blob/master/thirdparty/build_thirdparty.sh (both of these were originally patterned after Impala's build_thirdparty.sh script) -- this does install several libraries in the /installed directory.
2. I agree that benchmarks are a separate beast, and should be disabled by default
3. I'm not sure either. I would just build with -Wall and no other options on Mac unless the build fails. The tr1/tuple thing is annoying because googletest monkey patches std::tuple, causing much fun on C++11 / particular versions of XCode.
4. It might be nice to have a POC for an ADD_ARROW_BENCHMARK to go along with the ADD_ARROW_TEST cmake macro, so there's a pattern for adding new benchmark executables. Edit: Oops per item 4 I missed your other comment. Sounds good to me. We can create benchmarking fixtures as we go

[emkornfield]

OK, I think this is ready for review.

In regards to where we place google benchmark, I changed this to the install directory. It seems like there is a homebrew formula for it, but I could find any other package management systems that support it (I didn't look too hard).

I made the following changes which may or may not be controversial:

1. I refactored build-support/run-tests.sh to work for unit tests and other types of test (e.g. benchmark). I also think I fixed it so it is compatible with out of source builds.
2. Both benchmarks and unit tests get added to ctest if they are enabled. I added targets to control which ones are run. (new targets 'unittest' and 'runbenchmark')
3. By default benchmarks are disabled, but I have enabled them in travis CI to make sure any added benchmarks don't bit-rot.
4. I don't know if it counts as a fixture or not, but I did add a proof of concept benchmark test to verify everything works.
5. I tried to update the documentation to reflect the changes.

[wesm]

Cool, I can review in more detail tomorrow.

However, there is a conflicting aspect of this patch: in #28 I added an option to execute the unit tests with valgrind. Perhaps I should merge that patch (and let code reviews accumulate after the fact) so that you can rebase and make sure everything is working. Let me know what you think. We probably don't want to run the benchmarks through valgrind (on Travis CI, anyway).

Also, re: Travis CI, the benchmarks will in general be much slower to run than unit tests, so rather than running them by default, we perhaps should only build (to ensure no compilation errors) but do not run the benchmarks. Separately, I would like to find a benchmark reporting solution for continuous performance monitoring (not unlike my vbench hack http://pydata.github.io/vbench/)

[wesm]

Thanks for doing this; I wanted to do some refactoring to this once we got ASAN/TSAN setup (we'll see how relevant TSAN ends up being but ASAN builds will be useful)

[wesm]

I quickly reviewed this; everything looks good with the exception of the question around how (and if at all) the benchmarks are run in Travis CI.

I'm AFK rest of the day but will check this out and see what the CLI experience of running the benchmarks is like.

Small thing: can you make the PR title "ARROW-28:" to fulfill the wishes of the patch merge tool?

[wesm]

Per the Travis CI question: on the latest build, I see:

Label Time Summary:
benchmark    =   7.59 sec
unittest     =   0.81 sec

My only other question (which doesn't have to be resolved here) is whether we should co-locate the benchmarks in the same directory as the code and unit tests (versus having a top-level directory with all the benchmarks, or something similar).

[emkornfield]

Just building the benchmarks and not running them in travis CI seems OK as long as we can find a solution for the performance monitoring. I will make that change (hopefully I will have time for it tomorrow).

I'm also happy to handle the merge conflict however you want. If you are waiting on a review for #28 I can try to take a look at that tomorrow as well (it looked like you were waiting on some other people to comment on it though).

[emkornfield]

Title changed.

[emkornfield]

On the location of benchmarks. I don't have a strong preference. I think as long as we are colocating tests and code in the same directory, I don't think there is a strong reason for separating benchmarks. If we start developing end-to-end benchmarks we can reconsider if those belong in there own directory (I would imagine it would be the same for cross language integration tests).

typo generatic

Cool, I will wait for you to take a look at #28 (it's a large patch but contains a lot of refactoring / code cleaning, so don't feel obligated to look at all of it) before merging. I do want people to scrutinize the metadata IDL (and approach to shared memory interactions, generally) but I don't think the merge should be blocked on resolving open design questions there.

On the PR title, it needs to start exactly with "ARROW-28:", see https://github.com/apache/arrow/blob/master/dev/merge_arrow_pr.py#L221

Hopefully the title is fixed now. I should get the review of #28 and change to only run in travis later tonight.

Good now, thank you =)

+1, thank you very much.

I asked the JIRA admins to allow anyone to assign issues, so you should be able to go back and self-assign this and other JIRAs you've worked on to yourself