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

Make PearlDiver threads configurable #1017

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@CodeMonkeySteve
Copy link
Contributor

CodeMonkeySteve commented Sep 24, 2018

Description

Add configuration variable for the number of PoW threads, for manual control.

Fixes #1016

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

Deployed to production IOTA node, verified specified number of threads were used by CPU load.

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (where?)
  • New and existing unit tests pass locally with my changes
@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Oct 22, 2018

Hey @CodeMonkeySteve,

Thanks for the PR!
The reason it wasn't merged until now is because it has a conflict. You are using the old configuration design. By looking at the dates I see you probably didn't update your local dev branch since #910
was merged since August 16.

If you decide to do the neccessary changes, please also create a new PearlDiverConfig interface.

1 similar comment
@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Oct 22, 2018

Hey @CodeMonkeySteve,

Thanks for the PR!
The reason it wasn't merged until now is because it has a conflict. You are using the old configuration design. By looking at the dates I see you probably didn't update your local dev branch since #910
was merged since August 16.

If you decide to do the neccessary changes, please also create a new PearlDiverConfig interface.

@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Oct 22, 2018

Hey @CodeMonkeySteve,

Thanks for the PR!
The reason it wasn't merged until now is because it has a conflict. You are using the old configuration design. By looking at the dates I see you probably didn't update your local dev branch because #910
was merged since August 16.

If you decide to do the neccessary changes, please also create a new PearlDiverConfig interface.

@iotaledger iotaledger deleted a comment from codacy-bot Oct 30, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 30, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 30, 2018

@CodeMonkeySteve

This comment has been minimized.

Copy link
Contributor

CodeMonkeySteve commented Oct 30, 2018

I added the PearlDiver config, mimicking the other config, but there doesn't seem to be any effect from setting POW_THREADS in the INI file. Am I missing something? Is there something more needed for INI support? Has the format of that file changed?

Any insight would be greatly appreciated (I am not really a Java developer, as you may have noticed).

@iotaledger iotaledger deleted a comment from codacy-bot Oct 31, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Not bad :-)

Do the changes and try the INI file again :-)


@JsonProperty
@Parameter(names = "--pow-threads", description = PearlDiverConfig.Descriptions.POW_THREADS)
protected void setPoWThreads(int powThreads) {

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 1, 2018

Member

change to setPowThreads

Suggested change Beta
protected void setPoWThreads(int powThreads) {
protected void setPowThreads(int powThreads) {

Jackson knows how to translate the name to snake_case. This is why ini wasn't working

This comment has been minimized.

@CodeMonkeySteve

CodeMonkeySteve Nov 1, 2018

Contributor

Ah, thanks so much, that fixed it.

Show resolved Hide resolved src/main/java/com/iota/iri/conf/BaseIotaConfig.java Outdated
/**
* Configurations for PearlDiver proof-of-work hasher.
*/
public interface PearlDiverConfig extends Config {

This comment has been minimized.

@GalRogozinski

GalRogozinski Nov 1, 2018

Member

I prefer PowConfig

This comment has been minimized.

@CodeMonkeySteve

CodeMonkeySteve Nov 1, 2018

Contributor

Er, you originally asked for PearlDiverConfig. I think that's more appropriate, as there may be other config fields added later.

This comment has been minimized.

@GalRogozinski
Show resolved Hide resolved src/main/java/com/iota/iri/conf/PearlDiverConfig.java Outdated
Show resolved Hide resolved src/main/java/com/iota/iri/conf/PearlDiverConfig.java Outdated
@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Great!

@GalRogozinski GalRogozinski merged commit 51ebe60 into iotaledger:dev Nov 1, 2018

1 of 2 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jakubcech

This comment has been minimized.

Copy link
Contributor

jakubcech commented Nov 1, 2018

Awesome stuff, thank you @CodeMonkeySteve!

@CodeMonkeySteve CodeMonkeySteve deleted the CodeMonkeySteve:pow_threads branch Nov 1, 2018

@CodeMonkeySteve

This comment has been minimized.

Copy link
Contributor

CodeMonkeySteve commented Nov 1, 2018

Happy to help. It's also in my own best interest, as I'm creating >10k transactions per day, so I need to make use of all the CPU cores I have.

@GalRogozinski GalRogozinski referenced this pull request Dec 22, 2018

Merged

Release v1.5.6 #1251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment