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.
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
Introspection #58
Introspection #58
Changes from 7 commits
12b9584
c017d72
23061ef
9469776
542738c
fedb5f8
7423cef
4ad9c10
b9a0c7c
2a96fe3
55703cb
042bfce
2b6b369
8219da3
33df973
b9e75c0
3fb4626
0ac407a
6cc1e96
1fa893e
5d1eb27
f56f340
1e61748
92116b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth using https://docs.rs/llvmint/0.0.3/llvmint/fn.readcyclecounter.html, as that will automatically do the right thing for each arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds really useful for more obscure platforms!
We should only add the dependency for this feature flag though (and maybe even only for the platforms we can not reach otherwise?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sure! Didn't know about the llvm intrinsic. If we're okay with the
llvmint
dependency then I'm fine with using it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original thought was to add those specific instructions similar to Google's benchmark: https://github.com/google/benchmark/blob/master/src/cycleclock.h
I think that would mitigate the external dependency just for one instruction per arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that would require asm... which is not stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's go with
libc
, if available for this archllvmint
dependency otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or since it's a feature flag anyway, just llvmint is fine too, if it's too much work otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a fix using
llvmint::readcyclecounter
. Looks like the feature needed requiresnightly
. I think you guys are wanting to stay onstable
as far as I'm aware, which would mean neither this or theasm!
fix would work. I guess we could also requirenightly
for theperf_stats
feature as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit from the last comment.. i had the wrong
cfg_attr
. I've ripped out only the needed extern fromllvmint
so we don't need to add that dependency directly tosrc/cpu.rs
and it seems to work fine now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domenukk porcodio, now we have to release again because this line was commented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be needed, I agree