log: Fix log message for -par=1 by hebasto · Pull Request #17325 · bitcoin/bitcoin

hebasto · 2019-10-30T20:30:18Z

practicalswift · 2019-10-30T20:43:23Z

Concept ACK

laanwj · 2019-10-30T20:44:27Z

src/init.cpp

Concept ACK, my only comment would be ~~that -par=0 and -par=1 do the same thing (right?)~~, but log different messages.

Not exactly:

bitcoin/src/init.cpp

Line 380 in 08e2947

gArgs.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)",

Wait, I'm not really talking about the input argument, but the already-processed value we end up with here.

It's doing the same thing in case of nScriptCheckThreads==0 and nScriptCheckThreads==1 so should log the same thing.

After this code

bitcoin/src/init.cpp

Lines 1064 to 1071 in 08e2947

// -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency

nScriptCheckThreads = gArgs.GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS);

if (nScriptCheckThreads <= 0)

nScriptCheckThreads += GetNumCores();

if (nScriptCheckThreads <= 1)

nScriptCheckThreads = 0;

else if (nScriptCheckThreads > MAX_SCRIPTCHECK_THREADS)

nScriptCheckThreads = MAX_SCRIPTCHECK_THREADS;

is executed in AppInitParameterInteraction(), nScriptCheckThreads==1 is an impossible value.

Ok, so it will never log Using 1 threads concurrently for script verification?

practicalswift · 2019-10-31T12:09:25Z

src/init.cpp

Something feels a bit off with the wording here. What about "Using one thread for script verification"?

It's not just any thread though, it's the main thread. No additional threads are spawned. I think that's what he's trying to convey.

What about:

"Validation concurrency disabled: using the main thread for script validation."

Isn't this the thread:

bitcoin/src/init.cpp

Line 1267 in feb1a8c

threadGroup.create_thread(std::bind(&TraceThread<CScheduler::Function>, "scheduler", serviceLoop));

?

That spawns the scheduler thread, which is unrelated to script verification.

On -par=1, it will inline the calls into the current thread. See also:

git grep '\-par=' test

maflcko · 2019-10-31T14:35:04Z

src/init.cpp

Suggested change

LogPrintf("Using the only thread for script verification\n");

LogPrintf("Script verification is done in the main thread\n");

But script verification is always done in the main thread (as well), the other threads are only additional 😄

"Script verification is done in the main thread only (no validation concurrency)"

or "Script verification uses N additional threads" where N can be 0… it doesn't have to be a whole story

Yeah, let's make it a one-line "fix"

shortest would be

LogPrintf("Script verification uses %d additional threads\m", std::max(nScriptCheckThreads-1, 0));

True, it is shorter than conditional operator ;)

~~I don't think this should be nScriptCheckThreads-1. nScriptCheckThreads is the number of threads that are dedicated to script checking, so:~~

~~if nScriptCheckThreads >= 2, then log "Script verification uses {nScriptCheckThreads} additional threads"~~

~~if nScriptCheckThreads == 0, then script checking is done on the main thread so log "Script verification uses 0 additional threads"~~

~~nScriptCheckThreads cannot == 1 because of the logic in init.cpp~~

~~so you just want:~~

~~LogPrintf("Script verification uses %d dedicated threads\m", nScriptCheckThreads);~~

~~The log is already essentially correct. I think you just need to add the word 'dedicated' or 'additional' (I prefer 'dedicated').~~

@sipa has corrected my misunderstanding below.

That's not correct; it should be LogPrintf("Script verification uses %d dedicated threads\m", nScriptCheckThreads - 1); then, as nScriptCheckThreads includes the main thread.

@sipa The std::max(nScriptCheckThreads-1, 0) bound is there because nScriptCheckThreads might be zero at this point.

Co-authored-by: Wladimir J. van der Laan <[email protected]>

hebasto · 2019-10-31T17:01:01Z

All comments have been addressed.

jnewbery · 2019-10-31T22:45:23Z

Tested ACK 8a0ca5e

If we wanted to be completely explicit we could log "Script verification uses main thread and %d dedicated threads\n"

laanwj · 2019-11-01T09:47:45Z

ACK 8a0ca5e

If we wanted to be completely explicit we could log "Script verification uses main thread and %d dedicated threads\n"

I know bitcoin core is lacking in user documentation, but I don't think the debug log is the place for it 😄
The value is now correct, which is good for troubleshooting.

8a0ca5e log: Fix log message for -par=1 (Hennadii Stepanov) Pull request description: Fix #17139 ACKs for top commit: jnewbery: Tested ACK 8a0ca5e laanwj: ACK 8a0ca5e Tree-SHA512: 09f5416c00cd3e4f85cd9040267dc825d5c5623f8903e9d2373013686dce7f6b3ba573648787adc1d58e6f1d5012e26c78700d585273d4b508cb25312b3fa06b

8a0ca5e log: Fix log message for -par=1 (Hennadii Stepanov) Pull request description: Fix bitcoin#17139 ACKs for top commit: jnewbery: Tested ACK 8a0ca5e laanwj: ACK 8a0ca5e Tree-SHA512: 09f5416c00cd3e4f85cd9040267dc825d5c5623f8903e9d2373013686dce7f6b3ba573648787adc1d58e6f1d5012e26c78700d585273d4b508cb25312b3fa06b

- doc: fix git add argument bitcoin#18513 - build: Fix libevent linking for bench_bitcoin binary bitcoin#18397 - script: fix SCRIPT_ERR_SIG_PUSHONLY error string bitcoin#18412 - doc: Comment fix merkle.cpp bitcoin#18379 - log: Fix UB with bench on genesis block bitcoin#15283 - test: Fix mining to an invalid target + ensure that a new block has the correct hash internally bitcoin#18350 - Fix missing header in sync.h bitcoin#18357 - refactor: Fix implicit value conversion in formatPingTime bitcoin#18260 - Fix .gitignore policy in build_msvc directory bitcoin#18108 - build: Fix behavior when ALLOW_HOST_PACKAGES unset bitcoin#18051 - test: Fix p2p_invalid_messages failing in Python 3.8 because of warning bitcoin#17931 - qa: Fix double-negative arg test bitcoin#17893 - build: Fix configure report about qr bitcoin#17547 - log: Fix log message for -par=1 bitcoin#17325 - bench: Fix negative values and zero for -evals flag bitcoin#17267 - depends: fix boost mac cross build with clang 9+ bitcoin#17231 - doc: Fix broken bitcoin-cli examples bitcoin#17119 - doc: fix Makefile target in benchmarking.md bitcoin#17081 - contrib: fix minor typos in makeseeds.py bitcoin#17042 - test: Fix Python Docstring to include all Args. bitcoin#17030 - doc: Fix some misspellings bitcoin#17351 - doc: Doxygen-friendly script/descriptor.h comments bitcoin#16947 - doc: Fix doxygen errors bitcoin#17945 - doc: Doxygen-friendly CuckooCache comments bitcoin#16986 - doc: Add to Doxygen documentation guidelines bitcoin#17873 - depends: Consistent use of package variable bitcoin#17928 - init: Replace URL_WEBSITE with PACKAGE_URL bitcoin#18503 - doc: Add missed copyright headers bitcoin#17691

8a0ca5e log: Fix log message for -par=1 (Hennadii Stepanov) Pull request description: Fix bitcoin#17139 ACKs for top commit: jnewbery: Tested ACK 8a0ca5e laanwj: ACK 8a0ca5e Tree-SHA512: 09f5416c00cd3e4f85cd9040267dc825d5c5623f8903e9d2373013686dce7f6b3ba573648787adc1d58e6f1d5012e26c78700d585273d4b508cb25312b3fa06b

laanwj reviewed Oct 30, 2019

View reviewed changes

fanquake added the Utils/log/libs label Oct 30, 2019

practicalswift reviewed Oct 31, 2019

View reviewed changes

maflcko reviewed Oct 31, 2019

View reviewed changes

hebasto force-pushed the 20191030-fix-par-log branch from 3ee5d33 to 417c910 Compare October 31, 2019 16:46

log: Fix log message for -par=1

8a0ca5e

Co-authored-by: Wladimir J. van der Laan <[email protected]>

hebasto force-pushed the 20191030-fix-par-log branch from 417c910 to 8a0ca5e Compare October 31, 2019 16:59

laanwj merged commit 8a0ca5e into bitcoin:master Nov 1, 2019

hebasto deleted the 20191030-fix-par-log branch November 1, 2019 10:12

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

	// -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency
	nScriptCheckThreads = gArgs.GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
	if (nScriptCheckThreads <= 0)
	nScriptCheckThreads += GetNumCores();
	if (nScriptCheckThreads <= 1)
	nScriptCheckThreads = 0;
	else if (nScriptCheckThreads > MAX_SCRIPTCHECK_THREADS)
	nScriptCheckThreads = MAX_SCRIPTCHECK_THREADS;

	LogPrintf("Using the only thread for script verification\n");
	LogPrintf("Script verification is done in the main thread\n");

Conversation

hebasto commented Oct 30, 2019

Uh oh!

practicalswift commented Oct 30, 2019

Uh oh!

laanwj Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 31, 2019

Uh oh!

jnewbery commented Oct 31, 2019

Uh oh!

laanwj commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj Oct 30, 2019 •

edited

Loading

laanwj Oct 31, 2019 •

edited

Loading

practicalswift Oct 31, 2019 •

edited

Loading

laanwj Oct 31, 2019 •

edited

Loading

laanwj Oct 31, 2019 •

edited

Loading

jnewbery Oct 31, 2019 •

edited

Loading

sipa Oct 31, 2019 •

edited

Loading