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

Move logging off the main thread #16325

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Move logging off the main thread #16325

merged 3 commits into from
Apr 1, 2020

Conversation

jmalanen
Copy link
Contributor

@jmalanen jmalanen commented Mar 20, 2020 •
edited
Loading

Fixes: mapbox/mapbox-gl-native-team/issues/224

@julianrex
Copy link
Contributor

What will this mean for platform SDKs that implement observers? /cc @captainbarbosa

@jmalanen jmalanen force-pushed the jmalanen-log branch 7 times, most recently from b612466 to e843aff Compare March 23, 2020 20:32
@jmalanen jmalanen requested a review from pozdnyakov March 24, 2020 07:32
@jmalanen jmalanen marked this pull request as ready for review March 24, 2020 07:32
@jmalanen jmalanen requested a review from alexshalamov March 24, 2020 07:32
alexshalamov
alexshalamov approved these changes Mar 24, 2020
View reviewed changes
src/mbgl/util/logging.cpp
int64_t code,
const std::string& msg,
const optional<std::string>& threadName) {
std::lock_guard<std::mutex> lock(mutex);
if (currentObserver && severity != EventSeverity::Debug &&
currentObserver->onRecord(severity, event, code, msg)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a breaking behavior change. Where thread safety should be implemented? On client side or we need to post currentObserver->onRecord invocation on the thread where setObserver was called?

Copy link
Contributor

Choose a reason for hiding this comment

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

What was the outcome of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of the developer providing the thread/queue for logging, rather than one being created, but that feels like a possible follow-on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This question was more directed to Android and iOS, and both have now confirmed that the log observers are thread safe.

Regarding the developer providing the thread/queue, it can be a possible follow-up in case there's a need for it.

@alexshalamov alexshalamov self-requested a review March 24, 2020 08:40
@alexshalamov
Copy link
Contributor

@julianrex @tobrun if you use log observers, are they thread safe?

@tobrun
Copy link
Member

tobrun commented Mar 24, 2020

@julianrex @tobrun if you use log observers, are they thread safe?

Android, yes

pozdnyakov
pozdnyakov reviewed Mar 24, 2020
View reviewed changes
include/mbgl/util/logging.hpp Outdated Show resolved Hide resolved
src/mbgl/util/logging.cpp Outdated Show resolved Hide resolved
src/mbgl/util/logging.cpp Outdated Show resolved Hide resolved
@jmalanen jmalanen requested a review from pozdnyakov March 24, 2020 12:40
alexshalamov
alexshalamov approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm, maybe worth waiting for feedback from iOS folks

@jmalanen jmalanen self-assigned this Mar 24, 2020
@jmalanen jmalanen requested a review from julianrex March 24, 2020 15:23
@chloekraw
Copy link
Contributor

agreed, cc @mapbox/maps-ios @1ec5 to review

@chloekraw chloekraw requested a review from knov March 24, 2020 20:02
@jmalanen jmalanen requested a review from 1ec5 March 25, 2020 07:20
@julianrex
Copy link
Contributor

Is the primary use-case for useLogThread == false debugging?

@julianrex
Copy link
Contributor

Can we merge this after 1.5.0 please?

@chloekraw chloekraw added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Mar 25, 2020
@jmalanen
Copy link
Contributor Author

Is the primary use-case for useLogThread == false debugging?

@julianrex It's needed for GL native unit tests. Some tests are counting the number of logs to decide pass/fail so the second commit in this PR disables the logging thread for those tests.

@julianrex
Copy link
Contributor

Thanks @jmalanen - it would be a good option to make this configurable, especially to the end developer - does this allow for that?

@jmalanen
Copy link
Contributor Author

@julianrex It is already configurable. The developer can call Log::useLogThread(false) in case they don't want to use the logging thread.

@chloekraw chloekraw added needs changelog Indicates PR needs a changelog entry prior to merging. and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 26, 2020
@jmalanen
Copy link
Contributor Author

  • Added changelog entry
  • Release 1.5.0 was made so this is OK to merge now

@jmalanen jmalanen removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 27, 2020
@jmalanen
Copy link
Contributor Author

@1ec5 @julianrex Can you please review this from iOS point of view?

Juha Alanen added 3 commits March 31, 2020 11:08
[core] Move logging off the main thread
766f797
[test] Disable logging thread
462bcdd 
some tests are counting the number of logs to determine pass/fail
so disable separate logging thread for them.
Update changelog
90c7166
julianrex
julianrex reviewed Mar 31, 2020
View reviewed changes
test/src/mbgl/test/fixture_log_observer.cpp
@@ -60,6 +60,7 @@ size_t FixtureLog::Observer::count(const Message& message, bool substring) const

FixtureLog::FixtureLog() : observer(new FixtureLogObserver(this)) {
Log::setObserver(std::unique_ptr<Log::Observer>(observer));
Log::useLogThread(false);
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 a test where useThread is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All other tests that don't count the number of log messages have useThread == true

@julianrex
Copy link
Contributor

Looks good from an iOS pov - our default logging observer is thread-safe.

A few questions, but shouldn't block this PR:

  • Before this PR, was it possible for logging to occur off the main thread?
  • If useThread == false does that mean that all logs are on the main thread, or just the thread where the log was initiated?
  • Are there any use cases where we should expect multiple Log objects (not just the singleton)?

@jmalanen
Copy link
Contributor Author

jmalanen commented Apr 1, 2020

Looks good from an iOS pov - our default logging observer is thread-safe.

@julianrex Thank you for the review! Merging the PR based on this.

A few questions, but shouldn't block this PR:

  • Before this PR, was it possible for logging to occur off the main thread?

No.

  • If useThread == false does that mean that all logs are on the main thread, or just the thread where the log was initiated?

The thread where the log was initiated.

  • Are there any use cases where we should expect multiple Log objects (not just the singleton)?

I am currently not aware of such use cases, but it could be another follow-up feature in case there's a need for that.

@jmalanen jmalanen merged commit 09a5ee2 into master Apr 1, 2020
@jmalanen jmalanen deleted the jmalanen-log branch April 1, 2020 08:17
@1ec5 1ec5 mentioned this pull request Apr 18, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@julianrex julianrex julianrex left review comments

@alexshalamov alexshalamov alexshalamov approved these changes

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@knov knov Awaiting requested review from knov

@1ec5 1ec5 Awaiting requested review from 1ec5

Assignees

@jmalanen jmalanen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jmalanen @julianrex @alexshalamov @tobrun @chloekraw @pozdnyakov