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.

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

Implement a high-level API and crate for ittapi #45

Merged
merged 7 commits into from
Feb 23, 2022

Conversation

bnjbvr
Copy link
Contributor

@bnjbvr bnjbvr commented Feb 11, 2022

This is a work-in-progress of what a high-level API and crate could look like for ittapi. This creates a small workspace in ittapi-rs with two sub-crates:

  • ittapi-sys, which contains all the low-level bindings: it's the unmodified crate that's called ittapi-rs right now.
  • ittapi, which contains higher-level bindings, and what ought to be sufficient for wasmtime.

It would need to be cleaned up a bit, and commented, with examples, etc. But wanted to post early progress, if people wanted to try it out and confirm it works for their use case.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 11, 2022

@abrown
Copy link
Contributor

abrown commented Feb 11, 2022

cc: @jlb6740

@abrown
Copy link
Contributor

abrown commented Feb 11, 2022

Initial feedback:

  • the crate split makes sense to me
  • glancing around at the C documentation, I wonder if we should try to match the logical structure of the components there: e.g., jit::Method::load(...) -> Method, jit::notify_event(..., &Method), and over in the ITT part, itt::Domain::new(...) -> Domain, itt::task_begin(&Domain, ...), etc. I'm not saying we also can't have "do multiple things" helpers like here but it may help users to have an easy map from the C components to Rust structures--thoughts?

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 15, 2022

Good idea! Did just that, and now the API resembles much the one from the C documentation, with naming that's closed to the one from the C documentation. This looks like a mid-level API now, but that seems fine; additional helpers can be built on top of this to make it really really easy to use.

@bnjbvr bnjbvr marked this pull request as ready for review February 15, 2022 14:27
@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 15, 2022

Quite satisfied with the current state of this crate now, calling it ready for review 🎉

@jlb6740
Copy link
Contributor

jlb6740 commented Feb 18, 2022

@bnjbvr Hi .. so effectively this just hides the need to encapsulate calls with unsafe and other minor details to the api user, correct? Simplifies the ITTAPIs. How does someone who uses the abstraction know that a call is unsafe?

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

Had these comments pending and forgot to submit them.

@@ -0,0 +1 @@
../../
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the copy-c-library.ps1 script should also get an extra parent level due to this change.

MethodId(unsafe { ittapi_sys::iJIT_GetNewMethodID() })
}

/// Notifies any `EventType` to Vtune.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Notifies any `EventType` to Vtune.
/// Notifies any `EventType` to VTune.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a round of checking capitalization:

  • in Rust it's a bit uncommon to have two capital letters following each other, so kept Vtune, but I don't care too much about it.
  • used "VTune" in all comments though

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 21, 2022

@bnjbvr Hi .. so effectively this just hides the need to encapsulate calls with unsafe and other minor details to the api user, correct? Simplifies the ITTAPIs. How does someone who uses the abstraction know that a call is unsafe?

The whole purpose of such a high-level crate is to hide all the unsafety. In general, the -sys crate exposes mostly unsafe functions, and the high-level one wraps those unsafe functions into safe functions, assuming that most/all the calls will effectively work. It's the idiomatic way of wrapping low-level bindings in the Rust community, as far as I know.

@abrown abrown merged commit 83d6ca4 into intel:master Feb 23, 2022
@bnjbvr bnjbvr deleted the high-level-crate branch February 23, 2022 20:04
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.

None yet

3 participants