Skip to content

Additional Data benchmarks #21766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 10, 2019

Conversation

itaiferber
Copy link
Contributor

What's in this pull request?
Per @palimondo's suggestion, adds benchmarks covering worst-case Data.init<S>(_:)/Data.append<S>(_:) scenarios, as well as Data hashing.

Should go in for #21754 testing.

@itaiferber
Copy link
Contributor Author

@swift-ci Please smoke test

@itaiferber
Copy link
Contributor Author

@swift-ci Please benchmark

@palimondo
Copy link
Contributor

palimondo commented Jan 10, 2019

@itaiferber Could you please use the new benchmark naming convention, please? (Data.init.Sequence.UnderestimatedCount, etc.)

@itaiferber
Copy link
Contributor Author

@palimondo None of the other tests are named this way yet. I can make a pass to change them all if you prefer

@palimondo
Copy link
Contributor

Yes please. All newly added benchmarks should use that.

@itaiferber
Copy link
Contributor Author

@palimondo All names should now hopefully match convention

@itaiferber
Copy link
Contributor Author

@swift-ci Please benchmark

@itaiferber
Copy link
Contributor Author

@swift-ci Please smoke test

Copy link
Contributor

@palimondo palimondo left a comment

Choose a reason for hiding this comment

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

LGTM, with the following naming changes.

Copy link
Contributor

@palimondo palimondo left a comment

Choose a reason for hiding this comment

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

Sorry for the misunderstanding. We don't want the existing test names renamed, because we need them to maintain them for long-term performance tracking. It's OK to have mixed naming in the file for now. Only the newly added test names need to be changed.

@itaiferber
Copy link
Contributor Author

@palimondo What sort of long-term performance tracking is planned here? We feel very strongly about maintaining consistency here; if it's critical to leave the old benchmark names as-is at the moment, then I'll revert this renaming. If we can update them later, then we can do it en-masse like I've done here; if not, then we won't be maintaining inconsistent names forever.

@swift-ci

This comment has been minimized.

@palimondo
Copy link
Contributor

By long term tracking I meant the existing Apple-internal LNT, which is tracking performance changes. It's the reason @eeckstein objects to against changing names of existing benchmarks.

My current plan to whip the Swift Benchmarking Suite (SBS) up-to-shape is as follows:
When I finish the legacyFactor pass, which is lowering the loop multipliers, but still reports historically consistent runtimes, I'd like to add a legacyName to BenchmarkInfo. Then I'll do another pass through SBS, applying new naming convention. This would be used to double report results from Benchmark_O. Runtimes multiplied by legacyFactor under the legacyName and the new names with cleaned up results. This will allow us to migrate to cleaned-up SBS without disturbing the long-term performance tracking. At which point I'll clean up the names here in DataBenchmarks like you did.

@itaiferber
Copy link
Contributor Author

Okay, that sounds reasonable. If we're actually concretely planning on migrating the old names forward then I'm okay with things being briefly inconsistent.

@palimondo
Copy link
Contributor

palimondo commented Jan 10, 2019

@itaiferber It looks like you have too big loop multiplier on Data.*.Sequence. The DataAppendSequence had 100. You might want to leave that one unchanged and just add Data.append.Sequence.UnderestimatedCount with the same multiplier or less — depending on how big of an improvement the buffering brings (you should care about the runtime falling under 1000 μs after you apply that optimization). Same for Data.init.Sequence.*.

Data.hash.Medium needs a smaller multiplier. I'd go with 1_000.

(You can safely ignore the "wide range of memory" warnings — that heuristic is too sensitive at the moment.)

@itaiferber itaiferber force-pushed the data-additional-benchmarks branch from 052aa59 to 3952b73 Compare January 10, 2019 20:13
Co-Authored-By: itaiferber <itai@itaiferber.net>
Copy link
Contributor

@palimondo palimondo left a comment

Choose a reason for hiding this comment

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

LGTM! Benchmark Report should be just a green checkmark now…
(Data.append.Sequence.UnderestimatedCount will definitely overshoot 1000 μs, but that's OK)

@palimondo
Copy link
Contributor

@swift-ci please benchmark

@itaiferber
Copy link
Contributor Author

@swift-ci Please smoke test

@itaiferber
Copy link
Contributor Author

@swift-ci Please benchmark

@palimondo
Copy link
Contributor

palimondo commented Jan 10, 2019

Thank you for your patience with all my nitpicking Itai! I'm off to 😴💤 (10pm here).

@itaiferber
Copy link
Contributor Author

@swift-ci Please smoke test

@swift-ci
Copy link
Contributor

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
CharacterLiteralsLarge 97 108 +11.3% 0.90x
Added
Data.append.Sequence.ExactCount 212 213 212
Data.append.Sequence.UnderestimatedCount 5920 5966 5936
Data.hash.Empty 108 111 109
Data.hash.Medium 175 179 177
Data.hash.Small 307 310 308
Data.init.Sequence.ExactCount 183 187 184
Data.init.Sequence.UnderestimatedCount 5974 6054 6003

Code size: -O

TEST OLD NEW DELTA RATIO
Regression
DataBenchmarks.o 62543 72247 +15.5% 0.87x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
DataSubscriptSmall 31 25 -19.4% 1.24x
DataCreateEmpty 200 170 -15.0% 1.18x
DataCountSmall 28 25 -10.7% 1.12x
DataCountMedium 34 31 -8.8% 1.10x
RandomIntegersLCG 828 772 -6.8% 1.07x (?)
Added
Data.append.Sequence.ExactCount 210 210 210
Data.append.Sequence.UnderestimatedCount 5130 5308 5227
Data.hash.Empty 108 111 109
Data.hash.Medium 175 178 176
Data.hash.Small 307 310 308
Data.init.Sequence.ExactCount 184 189 186
Data.init.Sequence.UnderestimatedCount 5205 5289 5234

Code size: -Osize

TEST OLD NEW DELTA RATIO
Regression
DataBenchmarks.o 32955 41761 +26.7% 0.79x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
DataCopyBytesMedium 534 464 -13.1% 1.15x (?)
Added
Data.append.Sequence.ExactCount 38129 38829 38387
Data.append.Sequence.UnderestimatedCount 8532 8768 8612
Data.hash.Empty 177 180 179
Data.hash.Medium 183 187 186
Data.hash.Small 406 414 409
Data.init.Sequence.ExactCount 38226 38294 38256
Data.init.Sequence.UnderestimatedCount 8578 8661 8606
Benchmark Check Report
⚠️ Data.append.Sequence.UnderestimatedCount execution took at least 5835 μs.
Decrease the workload of Data.append.Sequence.UnderestimatedCount by a factor of 8 (10), to be less than 1000 μs.
⚠️ Data.init.Sequence.UnderestimatedCount execution took at least 5951 μs.
Decrease the workload of Data.init.Sequence.UnderestimatedCount by a factor of 8 (10), to be less than 1000 μs.
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB
--------------

@itaiferber itaiferber merged commit 1d10a6a into swiftlang:master Jan 10, 2019
@itaiferber itaiferber deleted the data-additional-benchmarks branch January 10, 2019 21:59
@itaiferber
Copy link
Contributor Author

The above tests should improve with a fix I'll push out briefly.

itaiferber added a commit to itaiferber/swift that referenced this pull request Jan 11, 2019
…nchmarks

Additional Data benchmarks

(cherry picked from commit 1d10a6a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants