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

Calculate and display hash count of PoW #73

Merged
merged 6 commits into from
Sep 27, 2018
Merged

Calculate and display hash count of PoW #73

merged 6 commits into from
Sep 27, 2018

Conversation

marktwtn
Copy link
Collaborator

@marktwtn marktwtn commented Sep 21, 2018 •
edited

Calculate and sum the total hash count mentioned in #47 in doThePoW function.

The FPGA part will be implemented by @ajblane .

The BUILD_BENCH option and PoW execution time for hash rate will be added in the future pull requests.

2018/09/23: The BUILD_BENCH option has been added in Makefile.

@furuame
Copy link
Member

furuame commented Sep 21, 2018

Why don't just integrate BUILD_BENCH option and measuring PoW execution time in this PR?

@marktwtn marktwtn mentioned this pull request Sep 21, 2018
Closed
@marktwtn
Copy link
Collaborator Author

Why don't just integrate BUILD_BENCH option and measuring PoW execution time in this PR?

Just want to make sure the way of getting hash times is agreed by everyone.
I might be too careful.

@jserv jserv requested a review from furuame September 21, 2018 09:22
jserv
jserv reviewed Sep 21, 2018
View reviewed changes
src/pow_avx.c Outdated Show resolved Hide resolved
furuame
furuame requested changes Sep 21, 2018
View reviewed changes
src/pow_avx.c Outdated Show resolved Hide resolved
@jserv
Copy link
Member

jserv commented Sep 22, 2018

I disagree with the way to place several printf inside PoW implementation. All core parts of dcurl should not contain verbose opertions over stdio. Instead, each function should only work as "black box" via certain interface specified by the public header(s). If users would like to benchmark, they can straightforwardly invoke utility function and manipulate stdio operations manually.

@marktwtn
Copy link
Collaborator Author

I'll remove the printf inside PoW implementation.
It does make me feel weird but I just print it out without any doubt.

Back to the original implementation, I was going to create a new API to get the hash times.
Like the API of getting the PoW result.
I haven't figured out other better methods to get the hash times.
Is this the right way to do it?
I'm a little confused now. I might misunderstand some previous comments given to me.

@jserv
Copy link
Member

jserv commented Sep 22, 2018 •
edited by marktwtn

Back to the original implementation, I was going to create a new API to get the hash times.
Like the API of getting the PoW result.

It sounds fine to me.

I haven't figured out other better methods to get the hash times.
Is this the right way to do it?
I'm a little confused now. I might misunderstand some previous comments given to me.

A Qt developer wrote the article The Little Manual of API Design 10 years ago, and it is still insightful.

Quoted from 4.11 Strive for property-based APIs:

Since the properties can be set in any order, it is usually necessary to use lazy initialization and other techniques in the implementation to avoid recomputing the whole object every time a property changes.

It is identically important to statistics in dcurl internals. To get statistics of hash rate, developers should be able to look up certain property via dedicated APIs. In this case, it would be great if there is something like bookkeeping as info in each implementation context.

@marktwtn marktwtn changed the title Calculate and display hash times of PoW Calculate and display hash count of PoW Sep 23, 2018
@marktwtn
Calculate the hash count of PoW
c5dae88 
Calculate the hash count mentioned in DLTcollab#47 in `doThePoW` function
and get it with `getHashCount` API.
@marktwtn
Copy link
Collaborator Author

I use force push to overwrite the old commits with the new ones.

Now the hash count can be obtained with the getHashCount API.
The calculation and summation are done in the doThePoW.

It would show the hash count value when building and executing test-pow.c with BUILD_BENCH option enabled.

@jserv
Copy link
Member

jserv commented Sep 23, 2018

Would it be meaingful if bench is replaced with stat, which stands for statistics and state?

@marktwtn
Copy link
Collaborator Author

Would it be meaingful if bench is replaced with stat, which stands for statistics and state?

I'm ok with both naming.
The naming BUILD_BENCH comes from the issue #47 .

@chenwei-tw what do you think?

@furuame
Copy link
Member

furuame commented Sep 23, 2018

Fine for me 👍

furuame
furuame requested changes Sep 24, 2018
View reviewed changes
Copy link
Member

@furuame furuame left a comment

Choose a reason for hiding this comment

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

Implement the API getPoWInfo

@jserv
Copy link
Member

jserv commented Sep 25, 2018

@marktwtn, File README.md should be modified accordingly. Then, I would approve this pull request.

@furuame
Copy link
Member

furuame commented Sep 25, 2018 •
edited

There is no getHashCount API in the original dev branch or mentioned in other commit. This 7a91620 commit message is really weird.

@marktwtn marktwtn force-pushed the hash-count branch 2 times, most recently from edd5fc8 to 42b7ffc Compare September 26, 2018 03:52
@marktwtn
Implement getPoWInfo API to get PoW-related information
2dc1d4c 
The `getPoWInfo` API would return the PoW-related information
such as hash count and PoW execution time,
instead of returning the hash count value only.
furuame
furuame reviewed Sep 26, 2018
View reviewed changes
tests/test-pow.c Outdated Show resolved Hide resolved
furuame
furuame requested changes Sep 26, 2018
View reviewed changes
tests/test-pow.c Outdated Show resolved Hide resolved
@marktwtn
Update README.md
262b893 
Update the `BUILD_STAT` option and testing files with the corresponding result.
furuame
furuame reviewed Sep 26, 2018
View reviewed changes
src/pow_fpga_accel.h Outdated Show resolved Hide resolved
@jserv jserv merged commit 8d95ec5 into DLTcollab:dev Sep 27, 2018
@furuame
Copy link
Member

furuame commented Sep 27, 2018

Oops, hash count in fpga implementation has not been implemented yet.

@ajblane
Copy link
Collaborator

ajblane commented Sep 27, 2018

I will send a pull request for hash count with the fpga accelerator.

@ajblane
Copy link
Collaborator

ajblane commented Sep 27, 2018

There are compile errors for the fpga-accelerated PoW (!) and I will fix it.

@jserv
Copy link
Member

jserv commented Sep 27, 2018

By the way, we shall set up CI for building all configurations including FPGA part.

@ajblane
Copy link
Collaborator

ajblane commented Sep 27, 2018

There are also run-time errors the fpga-accelerated PoW.

@marktwtn
Copy link
Collaborator Author

marktwtn commented Sep 27, 2018 •
edited

There are compile errors for the fpga-accelerated PoW (!) and I will fix it.

That's my bad. I did not notice it.
The warning would disappear when enabling BUILD_STAT option.

There are also run-time errors the fpga-accelerated PoW.

You can contact me on slack if you have any problem.

@ajblane
Copy link
Collaborator

ajblane commented Sep 27, 2018

@marktwtn Thanks.

@marktwtn marktwtn deleted the hash-count branch October 3, 2018 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jserv jserv jserv left review comments

@furuame furuame furuame requested changes

Assignees

@marktwtn marktwtn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marktwtn @furuame @jserv @ajblane