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

Remove global variable #348

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nazar-pc
Copy link

@nazar-pc nazar-pc commented Mar 6, 2023

Generally bad practice to use global variables and easily fixable in this case

@arvidn
Copy link
Contributor

arvidn commented Mar 18, 2023

I suspect that this will make the code run slower, because of the extra indirections. Is there a stronger argument than "generally bad practice"?

@nazar-pc
Copy link
Author

I don't think you'll be able to detect any difference in performance here. CPUs are crazy good at these kinds of patterns.

My context is that I was working on turning this CLI into a library and needed to call it potentially many times from the same process, so having global variables was undesirable. There is one more place with static variable in disk reads that would still be a blocker for that though.

I saw the opportunity to improve code a bit and decided to contribute back, but if it is not a welcome change that is fine too.

@arvidn
Copy link
Contributor

arvidn commented Mar 18, 2023

we have measured performance degradation with smaller changes in the past. If you can demonstrate that there's no slow-down, I think this is a fine change.
I think your use case is a pretty strong argument and might be worth a small slow-down. But I think it would have to be quantified (and not assumed or guessed)

@arvidn
Copy link
Contributor

arvidn commented Mar 18, 2023

on the other hand, given bladebit, perhaps we don't care so much about a performance degradation in chiapos.

@nazar-pc
Copy link
Author

I ran ProofOfSpace with tmpfs a few times on 13900k and took the best result out of a few runs.

Before:

k22 7.786
k25 65.272

After:

k22 7.752
k25 63.996

I'm not sure it became faster, but shouldn't be significantly slower either. My test was not super scientific, I have a bunch of things running in background on my machine.

@wallentx wallentx requested a review from fchirica March 22, 2023 06:42
@MumfMeisterT
Copy link

We're short on time to review this, but if you fork chiapos and it works well for you, we'll consider up-streaming it.

@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants