Skip to content
This repository was archived by the owner on Jun 11, 2026. It is now read-only.

Use std::tuple instead of ks::Tuple in entry points#955

Merged
dcrc2 merged 1 commit into
masterfrom
dcrc2/std-tuple-in-entry-points
Jul 20, 2021
Merged

Use std::tuple instead of ks::Tuple in entry points#955
dcrc2 merged 1 commit into
masterfrom
dcrc2/std-tuple-in-entry-points

Conversation

@dcrc2

@dcrc2 dcrc2 commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

This PR changes the entry points to use std::tuple, as this is supported directly by pybind11. This would mean that we could remove the specialization for ks::Tuple in knossos-pybind.h.

(Edited: The following comment was resolved by #961.)

One difficulty: we are optionally logging the entry point arguments using operator<<, but there is no operator<< defined for std::tuple. There are various ways we might fix this:

  1. Define operator<< for std::tuple. This invasion of std:: is technically illegal, but would probably work as long as no-one tries the same thing somewhere else.
  2. Define our own print function to use instead of operator<<.
  3. Log the values of the arguments after converting to ks types. This would work OK as things stand, but not once we are doing elementwise operations. (We wouldn't want to log inside the loop.)
  4. Just remove the logging now. As @toelli-msft pointed out on Generate C++ entry points #925 (comment) it doesn't really make sense to have it turned on any more, unless you're debugging a small example.

Any thoughts? (@awf?)

@toelli-msft

Copy link
Copy Markdown
Contributor

Can we be confident that this is fine from the point of view of performance? The reason for ks::Tuple existing at all is that it has better performance than std::tuple (under certain circumstances). Are there some tests or benchmarks we can put in place to make sure we don't degrade performance when switching to std::tuple here?

@dcrc2

dcrc2 commented Jul 19, 2021

Copy link
Copy Markdown
Contributor Author

Can we be confident that this is fine from the point of view of performance? The reason for ks::Tuple existing at all is that it has better performance than std::tuple (under certain circumstances). Are there some tests or benchmarks we can put in place to make sure we don't degrade performance when switching to std::tuple here?

I'm confident that any difference won't be measurable. The main reasons for this:

  • We're not supporting tensors-of-tuples in entry point signatures, so construction of std::tuple objects is at worst a constant-time overhead.
  • For each element of the tuple, we have to convert a python object to a C++ object, which involves (at the very least) a dynamic check of its type. I'd expect that the time spent in std::tuple access will be small compared to this (which is itself still constant-time).

Since the goal of this PR is to remove the ks::Tuple-binding code, I don't think we can maintain a benchmark which compares the performance without defeating the purpose of the PR. I can try running the existing benchmarks before and after but it'll be surprising if there's any observable difference.

@toelli-msft

Copy link
Copy Markdown
Contributor

Thanks for the explanation. I'm happy for you to proceed as you see fit.

@awf

awf commented Jul 19, 2021

Copy link
Copy Markdown
Contributor

Happy to remove the logging, and happy with PR in general.

@dcrc2
dcrc2 force-pushed the dcrc2/std-tuple-in-entry-points branch from 2207346 to 9259ee2 Compare July 20, 2021 07:44
@dcrc2
dcrc2 changed the base branch from master to dcrc2/remove-entry-point-logging July 20, 2021 07:45
@dcrc2
dcrc2 marked this pull request as ready for review July 20, 2021 07:45
@dcrc2
dcrc2 requested a review from awf July 20, 2021 07:45
@dcrc2
dcrc2 force-pushed the dcrc2/remove-entry-point-logging branch from f38ed46 to b193f56 Compare July 20, 2021 07:57
@dcrc2
dcrc2 force-pushed the dcrc2/std-tuple-in-entry-points branch from 9259ee2 to 47940d5 Compare July 20, 2021 07:58
Base automatically changed from dcrc2/remove-entry-point-logging to master July 20, 2021 12:36
@dcrc2
dcrc2 force-pushed the dcrc2/std-tuple-in-entry-points branch from 47940d5 to e8a7f37 Compare July 20, 2021 12:38
@dcrc2
dcrc2 merged commit d2bb79a into master Jul 20, 2021
@dcrc2
dcrc2 deleted the dcrc2/std-tuple-in-entry-points branch July 20, 2021 13:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants