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

feat(sync): print epoch statistics in_IBD instead of each block #3443

Closed
wants to merge 1 commit into from
Closed

feat(sync): print epoch statistics in_IBD instead of each block #3443

wants to merge 1 commit into from

Conversation

chanhsu001
Copy link
Contributor

@chanhsu001 chanhsu001 commented Jun 10, 2022 •
edited
Loading

What problem does this PR solve?

Problem Summary: Print 1st block in each epoch and previous epoch statistics information, when syncing node is in IBD

What is changed and how it works?

What's Changed:

  • add statistics structure in ChainService to record epoch's info counters
  • when syncing node is in IBD, only print 1st block of each epoch
  • print previous epoch statistics
  • make sure the adding code runs fast and minimal impact to performance

running output as:

2022-06-10 23:21:57.683 +08:00 ChainService INFO ckb_chain::chain  Epoch 156 Statis: blocks: 1800, blocks size: 1094335, txs: 1910, time: 10 secs
2022-06-10 23:21:57.683 +08:00 ChainService INFO ckb_chain::chain  block: 253182, hash: 0xe9e515bcb0e866dc987d03b737cbfef6ababc88515f2ca88049b773df54d3178, epoch: 157(0/1366), total_diff: 0x16665f819887d04f63, txs: 2
2022-06-10 23:22:05.138 +08:00 ChainService INFO ckb_chain::chain  Epoch 157 Statis: blocks: 1366, blocks size: 824881, txs: 1456, time: 7 secs
2022-06-10 23:22:05.138 +08:00 ChainService INFO ckb_chain::chain  block: 254548, hash: 0xaf531604ff01cdbbbf03325819edcc440a319cee1c9e5dad42bd7d0ff4c26611, epoch: 158(0/1800), total_diff: 0x168d77756fd5e476fe, txs: 1
2022-06-10 23:22:16.037 +08:00 ChainService INFO ckb_chain::chain  Epoch 158 Statis: blocks: 1800, blocks size: 1522519, txs: 1977, time: 10 secs
2022-06-10 23:22:16.037 +08:00 ChainService INFO ckb_chain::chain  block: 256348, hash: 0xee09df249f0d32404bf87524a45f8567f6ed674e8802e1fb8ced94e1a164a9ed, epoch: 159(0/1781), total_diff: 0x16b5c05c049bac55fd, txs: 1
  • not adding tx's cycles sum in epoch statistics, cause don't find a easy way to fetch it(cycles info in reconcile_main_chain(), but use immutable ChainService).
  • omit print epoch statistics information at point of OUT_OF_IBD, cause handling corner condition will add more checking in each block insert; it will slow the running of ckb. (IMHP)

Related changes

  • PR to update owner/repo:

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note 

Title Only: Include only the PR title in the release note.

@chanhsu001 chanhsu001 requested a review from a team as a code owner June 10, 2022 16:02
@chanhsu001 chanhsu001 requested review from zhangsoledad and removed request for a team June 10, 2022 16:02
quake
quake requested changes Jun 14, 2022
View reviewed changes
chain/src/chain.rs Outdated
Comment on lines 509 to 530
if self.shared.is_initial_block_download() {
self.update_print_statics(&block, &tip_header, total_difficulty.clone());
} else {
info!(
"block: {}, hash: {:#x}, epoch: {:#}, total_diff: {:#x}, txs: {}",
tip_header.number(),
tip_header.hash(),
tip_header.epoch(),
total_difficulty,
block.transactions().len()
);
}
Copy link
Member

Choose a reason for hiding this comment

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

suggest to move all log related code to a single fn, it can reduce duplicate code, for example info!("block: {}...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just move related info_block call into standalone functions, but not all functions can be merged into single function, due to message format.

chain/src/chain.rs Outdated
) {
let tip_epoch = tip_header.epoch();

if tip_epoch.index() == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

It is recommended to change the comparison logic to compare block number against epoch starting number plus epoch length, which will reduce the complexity of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good advice!
i refactor this part, please help to check.
i'm not sure about if the code is the best way to tell first_block/last_block number of each epoch.

quake
quake reviewed Jun 14, 2022
View reviewed changes
chain/src/chain.rs Outdated
cycles,
self.shared.consensus().max_block_cycles()
);
if !self.shared.is_initial_block_download() {
Copy link
Member

Choose a reason for hiding this comment

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

why disable this log in ibd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

original block printing is as:
block #12 info...........
[block verifier] block #12 info... cycles: ....

my previous thoughts was omit both block and verifier print, but if you suggest to keep it, and i also think cycles information cannot be fetch from database, mabey keep it in log file is also usable...so i keep it.

@chanhsu001 chanhsu001 force-pushed the perf_runlog_in_ibd branch from e6d609c to f04884d Compare June 15, 2022 08:14
@chanhsu001 chanhsu001 requested a review from quake June 15, 2022 08:26
@chanhsu001 chanhsu001 force-pushed the perf_runlog_in_ibd branch from f04884d to 78de4be Compare June 16, 2022 02:59
@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the s:wontfix Status: Wont fix label Aug 13, 2022
@stale stale bot closed this Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhangsoledad zhangsoledad Awaiting requested review from zhangsoledad zhangsoledad is a code owner automatically assigned from nervosnetwork/ckb-dev

@quake quake Awaiting requested review from quake

Assignees
No one assigned
Labels
s:wontfix Status: Wont fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chanhsu001 @quake