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

extend benchmark to make the db-sync mode and node-profiler configurable #11647

Merged
merged 1 commit into from
May 31, 2022

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented May 26, 2022

example output from the node profiler:

image

@lgtm-com

This comment was marked as outdated.

Copy link

@1-justinstarr 1-justinstarr left a comment

Choose a reason for hiding this comment

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

running

@arvidn arvidn force-pushed the node-benchmark branch 2 times, most recently from f230a3f to 4684702 Compare May 26, 2022 15:23
@arvidn arvidn requested a review from trepca May 27, 2022 12:48
@@ -2,13 +2,15 @@

run_benchmark() {
# shellcheck disable=SC2086
python ./tools/test_full_sync.py run $3 --profile --test-constants "$1" &
python ./tools/test_full_sync.py run $3 --profile --node-profiler --test-constants "$1" &
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it just occurred to me that combining --profile and --node-profiler here will cause two cProfile instances to be active at the same time. Is that supported by python? I don't recall anything stating it isn't..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does look like they clobber each other. I don't think it's useful to enable both.

mariano54
mariano54 previously approved these changes May 27, 2022
@@ -2,13 +2,15 @@

run_benchmark() {
# shellcheck disable=SC2086
python ./tools/test_full_sync.py run $3 --profile --test-constants "$1" &
python ./tools/test_full_sync.py run $3 --profile --node-profiler --test-constants "$1" &
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reduced performance from this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty minor impact on wall-time:

without --node-profiler: 116.05 s
with --node-profiler: 116.72 s

trepca
trepca previously approved these changes May 27, 2022
@arvidn arvidn dismissed stale reviews from trepca and mariano54 via 96d84ae May 27, 2022 15:28
@arvidn
Copy link
Contributor Author

arvidn commented May 27, 2022

I removed --node-profiler from the run_benchmark.sh script. My initial idea was to enable wall-clock profiling via --node-profiler to get a better understanding of where we wait. I'm not sure exactly how that will turn out right now though. I might have to build something bespoke for that.

@arvidn arvidn requested review from trepca and mariano54 May 28, 2022 20:26
Copy link
Contributor

@trepca trepca left a comment

Choose a reason for hiding this comment

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

lgtm

@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label May 29, 2022
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@wallentx wallentx merged commit d346ef8 into main May 31, 2022
@wallentx wallentx deleted the node-benchmark branch May 31, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants