[WIP] Update benchmark readme #17112

gottesmm · 2018-06-11T18:08:21Z

This is a WIP to update the README to the current state of the repository.

gottesmm · 2018-06-11T19:36:27Z

@natecook1000 Can I get your opinion on this... have I made it clearer?

gottesmm · 2018-06-11T19:37:11Z

Jceciliani · 2018-06-11T19:51:01Z

I cannot edit this file due to permissions. When trying to push to Github Desktop, there is a JS error not allowing it, any suggestions? Based on what I know, the content looks good, I just want to make some texts fixes here and there before a merge.

gottesmm · 2018-06-11T20:22:29Z

@Jceciliani I would just comment on the PR itself using github's review UI.

Jceciliani

These are a few of the text changes I would recommend for this file.

Jceciliani · 2018-06-11T20:26:47Z

benchmark/README.md

Recommended change to "of regression, micro, small, and medium sized benchmarks that are used"

Jceciliani · 2018-06-11T20:28:40Z

benchmark/README.md

"There are 3 ways to build the benchmark suite:"

Jceciliani · 2018-06-11T20:29:23Z

benchmark/README.md

"NOTE: There are various levels of support here. Specifically:"

Jceciliani · 2018-06-11T20:30:25Z

benchmark/README.md

"Non-macOS Darwin platforms today do not support the swiftpm build."

Jceciliani · 2018-06-11T20:34:33Z

benchmark/README.md

"This is not due to intention, but rather, more engineering time would be necessary to produce these builds."

I am not entirely sure what is trying to be said here.

"productization" maybe is something that we need to add to the lexicon. Basically the idea is that there are certain things that are "possible" to do with this software today that may either be fragile, unsupported, or not tested. Productization is changing the software so that it is easily usable by others.

Jceciliani · 2018-06-11T20:38:22Z

benchmark/README.md

"to be built" or "to build"

Jceciliani · 2018-06-11T20:40:49Z

benchmark/README.md

"SwiftBench will be a listed built product in Xcode. To find the path of"

Jceciliani · 2018-06-11T20:41:14Z

benchmark/README.md

"in the" at the end

Jceciliani · 2018-06-11T20:44:45Z

benchmark/README.md

sample, the test runs are significantly longer than system jitter. Due to this, we can"

Jceciliani · 2018-06-11T20:45:22Z

benchmark/README.md

"This is done by the low level benchmark driver. It will measure the time taken for"

gottesmm · 2018-06-11T21:02:38Z

Thanks! Let me take a look at this.

palimondo

I got these suggested changes. (I hope I’m using this review interface right…)

palimondo · 2018-06-11T20:19:13Z

benchmark/README.md

Why leave out this option from description?

This is on purpose. Iterations is an expert level feature. It confuses people as a concept and should only be progressively disclosed if necessary. This README is the right place to do that.

I’m confused, did you mean “This README isn’t the right place…”?

palimondo · 2018-06-11T20:35:09Z

benchmark/README.md

I don’t know what exactly is meant by “system jitter” here, but this description doesn’t match my experience with Swift Benchmark Suite. This whole business with measuring average time over N iterations is the root cause of unstable results. Maybe leave out the detailed reasons for the existence of this for now as it’s subject to change?

That is the intention behind why we do this, so it is appropriate. If/when we make any of the proposed changes, we can change this.

palimondo · 2018-06-11T20:51:17Z

benchmark/README.md

Xcode should be spelled with capital X, there are several instances of this typo.

palimondo · 2018-06-11T20:58:24Z

benchmark/README.md

I’d write all script and command names with backticks (build-script, cmake, swiftpm) consistently in the whole document. There are multiple places where this should be fixed.

swiftpm isn't a command. But I am fine with changing the rest.

palimondo · 2018-06-11T21:01:53Z

benchmark/README.md

How does one control the optimization level when building with swiftpm? Maybe an example here would help?

Thats reasonable.

palimondo · 2018-06-11T21:04:16Z

benchmark/README.md

Non-unstable? As in stable? Or maybe “Run all benchmarks (except those tagged .unstable) for 3 samples…”

I am also fine with this.

gottesmm · 2018-06-14T22:54:05Z

Pavol, I am not going to add the rest of that stuff. If you think it is important create a PR and it can be talked about there. I only have a limited amount of time to spend on this.

gottesmm · 2018-06-14T22:55:21Z

*and I need to move on. I am just waiting on some feedback from @natecook1000. But I don't want to add more content to this PR.

…ces. I added sections on running/building/removed cruft/using swiftpm to build the benchmarks standalone.

palimondo · 2018-06-28T09:10:23Z

benchmark/README.md

 This directory contains the Swift Benchmark Suite. This is meant to be a suite
 of regression, micro, small, and medium sized benchmarks that are used to
-validate the performance of the Swift compiler and standard library.
+validate the performance of the Swift compiler and standard library. There aretwo main steps to using


Typo: "aretwo" is missing a space

palimondo · 2018-07-09T14:04:08Z

benchmark/README.md

+    $ cmake -G Ninja \
+        -DSWIFT_EXEC=$SWIFT_BUILD_DIR/bin/swiftc \
+        -DSWIFT_LIBRARY_PATH=$SWIFT_BUILD_DIR/lib/macosx \
+        -DCLANG_EXEC=$LLVM_BUILD_DIR/bin/clang \


This one didn't work for me. I've set the both variables to point to the corresponding build directory, but the LLVM_BUILD_DIR isn't being picked up correctly:

CMake Error at cmake/modules/SwiftBenchmarkUtils.cmake:61 (message): Unable to find Clang driver Call Stack (most recent call first): cmake/modules/AddSwiftBenchmarkSuite.cmake:27 (runcmd) CMakeLists.txt:254 (configure_build)

which seem's to try to infer the CLANG_EXEC again from SWIFT_DARWIN_XCRUN_TOOLCHAIN. I think there's bug in AddSwiftBenchmarkSuite.cmake. Line 24 should IMO include if(NOT CLANG_EXEC) check similar the the if(NOT SWIFT_EXEC) on line 14.

gottesmm · 2018-07-10T17:54:18Z

I need to finish some stuff up today, but I am going to try and swing back around to this later today.

gottesmm · 2019-03-12T09:20:12Z

@palimondo realistically I am not going to have time to do this. If you want to do something like this go ahead. = ).

palimondo · 2019-03-12T09:49:22Z

@gottesmm Can you restore this branch and let me make a fork of it first, please?

palimondo · 2019-03-12T09:58:02Z

@gottesmm Nevermind. I have grabbed the raw of the modified README.md. I’m good. I’ll have a look at updating and simplifying it based on this excellent rewite of yours, eventually. Thank you!

gottesmm force-pushed the benchmark-readme-fixup branch 3 times, most recently from 2b88ee0 to 5857266 Compare June 11, 2018 19:33

gottesmm requested a review from natecook1000 June 11, 2018 19:36

Jceciliani suggested changes Jun 11, 2018

View reviewed changes

palimondo suggested changes Jun 11, 2018

View reviewed changes

gottesmm force-pushed the benchmark-readme-fixup branch 7 times, most recently from 93041e1 to 148dc28 Compare June 14, 2018 18:34

[benchmark] Update the benchmark README.txt file given current practi…

8f93f73

…ces. I added sections on running/building/removed cruft/using swiftpm to build the benchmarks standalone.

gottesmm force-pushed the benchmark-readme-fixup branch from 148dc28 to 8f93f73 Compare June 14, 2018 22:59

Update README.md

eaf9804

palimondo reviewed Jun 28, 2018

View reviewed changes

palimondo reviewed Jul 9, 2018

View reviewed changes

This was referenced Jul 11, 2018

[benchmark] Restore running benchmarks by ordinal numbers and related bugfixes #12415

Merged

[WIP] Report detailed build version from Benchmark drivers #17393

Closed

gottesmm closed this Mar 12, 2019

gottesmm deleted the benchmark-readme-fixup branch March 12, 2019 09:20

gottesmm restored the benchmark-readme-fixup branch March 12, 2019 09:50

[WIP] Update benchmark readme #17112

[WIP] Update benchmark readme #17112

Uh oh!

Conversation

gottesmm commented Jun 11, 2018

Uh oh!

gottesmm commented Jun 11, 2018

Uh oh!

gottesmm commented Jun 11, 2018

Uh oh!

Jceciliani commented Jun 11, 2018

Uh oh!

gottesmm commented Jun 11, 2018

Uh oh!

Jceciliani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jun 11, 2018

Uh oh!

palimondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palimondo Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Jun 14, 2018

Uh oh!

gottesmm commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palimondo Jun 11, 2018 •

edited

Loading

palimondo commented Mar 12, 2019 •

edited

Loading