Fix benchmark suite on 32-bit platforms #17268
Merged
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
The automatic scaling mechanism may end up with iteration counts greater than 2^31, leading to integer overflow.
Values returned by random APIs depend on the bitWidth of the underlying integer type; make sure we use a consistent integer width when expecting particular results.
|
@swift-ci please smoke test |
|
@swift-ci please smoke benchmark |
Build comment file:Optimized (O)Regression (3)
Improvement (59)
No Changes (384)
Hardware Overview |
|
Same! |
|
@swift-ci please smoke test linux platform |
|
@lorentey @gottesmm @eeckstein I'm curious about the possibility of scale to overflow on 32-bit system this commit addresses. How are using the Looking at the code, I would have thought the only potential for overflow is on line 395: scale = UInt(time_per_sample / elapsed_time)if the elapsed time is very low (worst case: 1 microsecond) it would take a manual setting of I'm asking because I'm about to refactor the surrounding code and plan to replace all the exotic type selections ( For the record, this is how I'm doing the estimates:
So the explicit use of |
|
There are some imperfect benchmarks in this suite that take effectively zero time, independent of the scale factor given to them. Most likely the 1 ... N loop gets optimized away entirely, and these benchmarks aren't measuring anything useful. So elapsed_time can be as low as a handful of nanoseconds. However, the overflows I work around here happened in the benchmarks themselves, when they multiply the scaling factor with their own constants, which can be as high as 10,000 (or even 100,000 in places). Without the artificial limit I added, some benchmarks do try to run more than 2^31 iterations on 32-bit platforms (e.g., the i386 simulator). |
|
There is nothing exotic about specifying the bitwidth of an integer. It's the appropriate thing to do in code that performs measurement and computation. Introducing platform specific behavior in the benchmark driver is not appropriate. |
|
@lorentey Oh I see: You're just putting an upper bound on the @atrick I agree that it is important to be specific about integer width when it serves a concrete purpose. But Int-type choices in many parts of the benchmark driver are rather arbitrary and obfuscate more than clarify. Most of what I've seen so far would work safely with the capacity of So, do you know of any internal use of the |
palimondo
added a commit
to palimondo/swift
that referenced
this pull request
Aug 31, 2018
Clarified the need for capping `numIters` according to the discussion at swiftlang#17268 (comment) The sampling loop is a hairy piece of code, because it’s trying to reuse the calibration measurement as a regular sample, in case the computed `numIters` turns out to be 1. But it conflicts with the case when `fixedNumIters` is 1, necessitating a separate measurement in the else branch… That was a quick fix back then, but its hard to make it clean. More thinking is required…
palimondo
added a commit
to palimondo/swift
that referenced
this pull request
Sep 14, 2018
Clarified the need for capping `numIters` according to the discussion at swiftlang#17268 (comment) The sampling loop is a hairy piece of code, because it’s trying to reuse the calibration measurement as a regular sample, in case the computed `numIters` turns out to be 1. But it conflicts with the case when `fixedNumIters` is 1, necessitating a separate measurement in the else branch… That was a quick fix back then, but its hard to make it clean. More thinking is required…
dcci
pushed a commit
that referenced
this pull request
Sep 17, 2018
Clarified the need for capping `numIters` according to the discussion at #17268 (comment) The sampling loop is a hairy piece of code, because it’s trying to reuse the calibration measurement as a regular sample, in case the computed `numIters` turns out to be 1. But it conflicts with the case when `fixedNumIters` is 1, necessitating a separate measurement in the else branch… That was a quick fix back then, but its hard to make it clean. More thinking is required…
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.
rdar://problem/41177686