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

Avoid doing unneeded logger work in Replication #7400

Merged
merged 1 commit into from Mar 1, 2024

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Mar 1, 2024

Most of the replication log statements do some work including memory allocations which are then thrown away if the log level it too high, so always check the log level first. A few places don't actually benefit from this, but it's easier to consistently check the log level every time.

This makes the obj-c bulk insert benchmark slightly faster than v13.27.0 rather than ~10% slower.

Most of the replication log statements do some work including memory
allocations which are then thrown away if the log level it too high, so always
check the log level first. A few places don't actually benefit from this, but
it's easier to consistently check the log level every time.
@tgoyne tgoyne requested a review from jedelbo March 1, 2024 00:21
@tgoyne tgoyne self-assigned this Mar 1, 2024
@cla-bot cla-bot bot added the cla: yes label Mar 1, 2024
Copy link

Pull Request Test Coverage Report for Build thomas.goyne_205

Details

  • 63 of 100 (63.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on tg/replication-logging at 90.9%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/realm/replication.cpp 58 95 61.05%
Totals Coverage Status
Change from base Build 2083: 90.9%
Covered Lines: 238311
Relevant Lines: 262169

💛 - Coveralls

@nirinchev
Copy link
Member

Would it make sense to adopt this as a general-purpose approach for logging - maybe wrap it in a macro that would expand to:

if (auto logger = would_log(*log_level*)) {
  logger->log(*log_category*, *log_level*, *message*);
}

Not sure if it would be an optimization across the board, but I can easily imagine other places either now or in the future, where we'd unnecessarily construct messages.

@jedelbo
Copy link
Contributor

jedelbo commented Mar 1, 2024

@nirinchev I think the issue here is that these activities are the ones most frequently used.

@jedelbo jedelbo merged commit 64b9591 into master Mar 1, 2024
39 checks passed
@jedelbo jedelbo deleted the tg/replication-logging branch March 1, 2024 12:26
@tgoyne
Copy link
Member Author

tgoyne commented Mar 1, 2024

The performance impact of unused logging is definitely something we'll need to keep an eye on as we add more of it, but so far this is the only place it's actually been measurable so I think it's still too early to try to build a generalized mitigation.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants