-
Notifications
You must be signed in to change notification settings - Fork 185
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
Vm benchmark tests #299
Vm benchmark tests #299
Conversation
pub const ITERATIONS_COUNT: &'static str = env!("ITERATIONS_COUNT"); | ||
|
||
/// size of sequence that should be compressed on each iteration | ||
pub const SEQUENCE_SIZE: &'static str = env!("SEQUENCE_SIZE"); |
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.
I think would be better to remove setting.rs
file and define
mod setting {
...
}
inside lib.rs.
Now it looks redundant.
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.
AlI measurement results of these test are completely depend on parameters that defined in settings.rs
file. So I think that it is better to have all adjusted parameters in separate file. Yes, there are two tests that have only one parameter but for foreign viewer it is more understandable what settings exactly have the current test.
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.
It isn't more understandable when you have to see one 2 files instead of 1. And especially when one file has only a single line.
A 'setting' module as a separated module on the top of the file is good enough readable for a foreign viewer, I suggest.
use reikna::prime; | ||
|
||
#[no_mangle] | ||
pub extern "C" fn main() -> u64 { |
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.
extern "C"
is not required and could be skipped.
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.
It is not so easy question. Based on this article it can be said that [no_mangle]
implies not only mangling absence but also participate in decision will this function be exported or not:
Ugh, linkage attributes are set on best effort basis. They can depend on 1) item being pub and being reachable from other crates 2) item being used in inline functions 3) #[no_mangle] 4) extern "ABI" 5) item being related to lang items 6) crate type - lib or exe. If the combination of all factors make an item likely to be used from the outside and linked to - let’s put an external linkage on it!
I think that this behavior is ugly but according to 4 part extern "C"
increases the chance that this function will exported. So i suggest to leave it on all function that should be exported.
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.
I investigated this question and that's what I dug:
- pub, extern and #[no_mangle] are required
- "C" is redundant
from here
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.
Hmm, it seems that cargo fmt
adds "C". There is a separate option force_explicit_abi
that by default is true in cargo fmt
.
Also, from this link it isn't clear does ABI take part in decision of exporting function. I think that "C" may be useful later because of Rust is too young language and many things can differ in future.
use num_traits::One; | ||
use std::ops::Sub; | ||
|
||
/// recursively computes a fibonacci number F_num for the given num |
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.
Docs require a first capital letter and dot at the end.
This reverts commit d77202f.
No description provided.