Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Disable log reloading by default #9966

Merged
6 commits merged into from Oct 8, 2021
Merged

Disable log reloading by default #9966

6 commits merged into from Oct 8, 2021

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Oct 7, 2021 •
edited

This disables the log reloading that was enabled by default. The problem
is that the log reload implementation of tracing is using a lock to
make the layer replaceable. This lock needs to be locked every time we
need to check if a particular target is enabled (assuming the log level
is high enough). This kills the performance when for example
sometarget=trace logging is enabled.

polkadot companion: paritytech/polkadot#4037

@bkchr
Disable log reloading by default
374f1b1 
This disables the log reloading that was enabled by default. The problem
is that the log reload implementation of `tracing` is using a lock to
make the layer replaceable. This lock needs to be locked every time we
need to check if a particular target is enabled (assuming the log level
is high enough). This kills the performance when for example
`sometarget=trace` logging is enabled.
@bkchr bkchr added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Oct 7, 2021
ordian
ordian approved these changes Oct 7, 2021
View reviewed changes
client/cli/src/config.rs Outdated Show resolved Hide resolved
@crystalin
Copy link
Contributor

Awesome, thank you @bkchr !

cheme
cheme approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

LGTM

@bkchr
Copy link
Member Author

bkchr commented Oct 8, 2021

bot merge

1 similar comment
@bkchr
Copy link
Member Author

bkchr commented Oct 8, 2021

bot merge

@ghost
Copy link

ghost commented Oct 8, 2021

Trying merge.

@ghost ghost merged commit b535922 into master Oct 8, 2021
@ghost ghost deleted the bkchr-disable-log-reloading-by-default branch October 8, 2021 11:46
@joao-paulo-parity joao-paulo-parity mentioned this pull request Oct 8, 2021
Merged
@dvdplm
Copy link
Contributor

dvdplm commented Oct 11, 2021

The problem
is that the log reload implementation of tracing is using a lock to
make the layer replaceable. This lock needs to be locked every time we
need to check if a particular target is enabled (assuming the log level
is high enough).

Did we open an issue for this upstream? Even if they can't fix it easily it should at least be clearly documented in tracing.

@bkchr
Copy link
Member Author

bkchr commented Oct 11, 2021

Did we open an issue for this upstream? Even if they can't fix it easily it should at least be clearly documented in tracing.

I did not open an issue for it.

Depending on what you see as a fix, I think we could improve the performance of it. But I also realized later that we didn't had "parking_lot" enabled for tracing-subscriber, maybe that would already had improved the performance.

@bkchr
Copy link
Member Author

bkchr commented Oct 11, 2021

tokio-rs/tracing#1632

@ghzlatarev ghzlatarev mentioned this pull request Nov 2, 2021
Merged
19 tasks
@github-actions github-actions bot mentioned this pull request Nov 2, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cheme cheme cheme approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bkchr @crystalin @dvdplm @cheme @ordian