[benchmark] SR-4780 Can not run performance tests that are not in precommit suite #9310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please review. |
palimondo
changed the title
palimondo
changed the title
|
@slavapestov Please review |
palimondo
changed the title
|
@practicalswift The DriverUtils.swift has a strange formatting, using two spaces indentation. Gardening opportunity? |
palimondo
changed the title
|
I got spooked by a failing build with this change on my machine and changed the title to [WIP]… There must have been some bug in Type Checker in the revision I based this originally on (2048bd5f). So now I've rebased on current tip and hope it works. The error was quite a head-scratcher: where the body of @slavapestov, @gottesmm, @dabrahams Please benchmark, smoke test and merge 🤠 |
|
@palimondo I'd be glad to clean up |
|
@practicalswift We're talking after this lands, yes? 🤤 |
|
@palimondo Sure :-) |
|
I still can not build this locally when rebased on badf215 😅 Same error as I've reported above. I don't see anything wrong with my code. When I take the Please smoke benchmark @practicalswift @dabrahams @gottesmm @atrick @slavapestov … If it turns out there's a bug in TypeChecker my code triggers, could you report that where appropriate? I'm signing off for tonight. Thank you! |
|
@swift-ci Please smoke benchmark |
Build comment file:Build failed before running benchmark. |
|
The CI build failed with the same compilation error like my local build. 🤦♂️ |
|
I've filed SR-4816 Type Checker is drunk. 🤷♂️🍻🥂🥃🍸🍷 |
|
The type checker isn't drunk, you're hitting up against SE-110. The type of this map is |
|
@CodaFi Let me get that straight, because this seems supremely messed up… Are you saying that I have been bumping into the root cause of this problem whole past week, but didn't realize this is what's going on here? There's SR-4745, where @natecook1000 asks about inability to destructure Is there a conspiracy against readable yet concise functional code? |
|
I've fixed the error, amended the commit and force pushed. Please smoke benchmark (& merge?). Playing with this test case revealed multiple strange behaviors in type checker, see SR-4816, so maybe we were both drunk! 🍻😅 |
gottesmm
reviewed
May 7, 2017
benchmark/utils/DriverUtils.swift
Outdated
| } | ||
|
|
There was a problem hiding this comment.
This should use enumerate. That is what you really want. Something like:
allTests.enumerate().map {
(index, value) in
let (key: name, value: function)) = value
...
}
There was a problem hiding this comment.
No I don't want enumerated, because it is off by one.
Again, this is something that lead me to file SR-4746 Let enumerated() count from specified number, pitch swift-evolution proposal but @airspeedswift says zip is the way to go. Rob Napier also independently refactored this code to use of zip when I asked him to improve on my style.
I don't normally summon the Council of Elrond about single bug fix, but this one irritated me so much I got my first article out of it...
... and some StackOverflow reputation for Python style conditional expression in Swift 3 🐍
benchmark/utils/DriverUtils.swift
Outdated
| .joined() | ||
|
|
||
| let included: Set<String> | ||
| if !filters.isEmpty { included = Set(filters) } else { |
There was a problem hiding this comment.
Why is there more indentation here?
There was a problem hiding this comment.
Because I had to manually space indent by 2 spaces, like an animal… and made a mistake.
Would you provide the rough consensus @practicalswift seeks to fix formatting in this file? [gardening]
benchmark/utils/DriverUtils.swift
Outdated
|
|
||
| let included: Set<String> | ||
| if !filters.isEmpty { included = Set(filters) } else { | ||
| if onlyPrecommit { included = Set(precommitTests.keys) } |
There was a problem hiding this comment.
Also, can you not use one line if else? I.e.:
if !filter.isEmpty {
...
} else {
...
}
There was a problem hiding this comment.
I could give you what you ask for… but I've obsessed about that particular detail a lot. Do you really think
let included: Set<String>
if !filters.isEmpty {
included = Set(filters)
} else {
if onlyPrecommit {
included = Set(precommitTests.keys)
} else {
included = Set(allTests.map { $0.key }) }
}is more clear way to express that we have 3 simple choices here? I don't think dogmatic adherence to one formatting style helps in this case.
There was a problem hiding this comment.
… but If you don't have allergy against ternary operator, my favorite version is:
let included =
!filters.isEmpty ? Set(filters) :
onlyPrecommit ? Set(precommitTests.keys) :
Set(allTests.map { $0.key })Just tell me which one you prefer and I'll amend and force push to get this over with!
There was a problem hiding this comment.
Ternary is great, though we put the colons at the beginning of the next line when the ternary has to be bbroken up. I don't know why Michael wants you to write the else that way; it's not a practice we've strictly adhered to in the standard library.
There was a problem hiding this comment.
Thanks! I've amended and force pushed. This fixes the indentation. I went with ternary formatted the way Dave suggested.
|
The nits were nits. I don't care that much about them. I think it makes the code easier to read. But lets get this in. |
|
Do I need to do something else to get this in? I thought it's smoke benchmark and merge time… |
|
🚬🏋️✔️❓🔀 |
|
@palimondo It makes sure that it builds correctly. That is all. |
|
@swift-ci smoke benchmark |
1 similar comment
|
@swift-ci smoke benchmark |
|
@palimondo Also due to force pushing, the initial smoke test didn't happen. So I am going to do the smoke benchmark and then a smoke test and merge |
Build comment file:Optimized (O) Regression (11)
Improvement (7)
No Changes (251)
Added (113)
Regression (2)
Improvement (6)
No Changes (261)
Added (113)
|
|
@palimondo Is it expected that so many benchmarks show up as added? |
|
That ain't right. It does not happen locally, while my branch is based on commit 86620aa. Let me see if someone modified Benchmark_Driver on ToT... |
|
Nope. It has something to do with how |
palimondo
changed the title
|
Ah, 💩. I don't understand why sometimes my forked branch gets some kind of update here on Github (shows as it needs to pull changes on my local working copy), and when I try to rebase it all goes to hell... |
palimondo
force-pushed
the
SR-4780
branch
4 times, most recently
from
a1db85e to
bda670d
Compare
SR-4780 Can not run performance tests that are not in precommit suite Modified driver to honor command line arguments when listing enabled tests. Fixed interaction between filters (positional arguments) and --run-all option. Benchmark_Driver lists available benchmarks with --run-all option when benchmarks or filters are specified.
palimondo
changed the title
|
I think I got this. |
|
@dabrahams Please review, benchmark, smoke test and merge. |
|
@swift-ci Please benchmark |
|
@swift-ci Please smoke test |
Build comment file:Optimized (O)Regression (5)
Improvement (6)
No Changes (270)
Unoptimized (Onone)Regression (1)
No Changes (280)
Hardware Overview |
|
@dabrahams Do you need a review here ( @gottesmm ?), or is it good to merge? |
|
LGTM. |
|
Thanks @dabrahams, @gottesmm and @CodaFi for your patience and help! This one took longer than expected ;-) |
This was referenced May 6, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves SR-4780 Can not run performance tests that are not in precommit suite.
Modified driver to honor command line arguments when listing enabled tests. Fixed interaction between filters (positional arguments) and
--run-alloption.Benchmark_Driverlists available benchmarks with--run-alloption when benchmarks or filters are specified.