Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[cxx-interop] Add benchmark for std::set conversion to a Swift collection #62648

Merged
merged 2 commits into from
Jan 24, 2023
Merged

[cxx-interop] Add benchmark for std::set conversion to a Swift collection #62648

merged 2 commits into from
Jan 24, 2023
+96 −5

Conversation

egorzhdan
Copy link
Contributor

@egorzhdan egorzhdan commented Dec 16, 2022 •
edited
Loading

This adds a benchmark for the C++ stdlib overlay functionality that converts C++ containers to Swift collections.

This also fixes a Python exception in the benchmarking engine that happened when no memory measurements were taken for a particular benchmark

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Dec 16, 2022
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@hyp
Copy link
Contributor

hyp commented Dec 16, 2022

what exactly are you benchmarking here, the cast from set to Array, or the Array iteration?

@egorzhdan
Copy link
Contributor Author

@hyp the intention is to benchmark the Array initializer, I added a for-in loop to make sure that it's not removed by the optimizer. Do you know a better way to benchmark this?

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch 2 times, most recently from f563401 to eaa8689 Compare December 17, 2022 11:46
@hyp
Copy link
Contributor

hyp commented Dec 17, 2022

You should just put the constructed array value into the blackHole. The compiler will not remove it then.

@egorzhdan
Copy link
Contributor Author

I haven't thought of this, thanks!

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch 3 times, most recently from bec725c to ea07771 Compare December 18, 2022 21:35
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch from ea07771 to fff5b04 Compare January 5, 2023 11:36
@egorzhdan egorzhdan changed the title [cxx-interop] Add benchmark for std::set iteration [cxx-interop] Add benchmark for std::set conversion to a Swift collection Jan 5, 2023
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch 3 times, most recently from 3ed77f4 to 35948d4 Compare January 13, 2023 19:12
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch 2 times, most recently from 185423f to 32cc95b Compare January 18, 2023 13:58
@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch from 32cc95b to 3311d98 Compare January 23, 2023 11:18
@egorzhdan egorzhdan marked this pull request as ready for review January 23, 2023 11:18
@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-set-bench branch from 3311d98 to f931de8 Compare January 23, 2023 13:38
@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan egorzhdan requested a review from hyp January 23, 2023 15:32
egorzhdan
egorzhdan commented Jan 24, 2023
View reviewed changes
benchmark/scripts/Benchmark_Driver
[r.mem_pages for r in i_series if r.mem_pages is not None]
for i_series in [select(measurements, num_iters=i) for i in [1, 2]]
]
memory_uses = [m for m in memory_uses if m]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tbkka could you please take a look at this part of the PR? This fixes a TypeError that was thrown out of this Python code when memory_uses is either empty or only contains empty elements.

cc @shahmishal

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine.

@egorzhdan egorzhdan merged commit 99c54ad into main Jan 24, 2023
@egorzhdan egorzhdan deleted the egorzhdan/cxx-set-bench branch January 24, 2023 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyp hyp

@zoecarver zoecarver

@tbkka tbkka

@shahmishal shahmishal

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@egorzhdan @hyp @tbkka