-
Notifications
You must be signed in to change notification settings - Fork 556
[CoreCLR] Remove unnecessary logging #10247
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reduces logging overhead and noise by removing or demoting various log messages across native and managed code, as well as updating the directory creation call.
- Removed or downgraded log messages in Android system initialization and host modules.
- Switched from direct mkdir calls to a call to create_directory for public directory creation.
- Removed redundant logs in the managed TypeManager.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/native/clr/runtime-base/util.cc | Updated to use create_directory instead of mkdir. |
src/native/clr/include/runtime-base/android-system.hh | Demoted log_warn to log_debug for directory creation. |
src/native/clr/host/host.cc | Removed log message on Host OnLoad. |
src/native/clr/host/host-jni.cc | Removed log message in JNI_OnLoad. |
src/native/clr/host/assembly-store.cc | Demoted a log_warn to log_debug with inline comment. |
src/Mono.Android/Java.Interop/TypeManager.cs | Removed log output for loaded type. |
Comments suppressed due to low confidence (1)
src/native/clr/runtime-base/util.cc:55
- Confirm that create_directory replicates the error handling behavior of mkdir; if there are any differences, consider adding an inline comment to clarify the change.
int ret = create_directory (dir.data (), 0777);
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow reviewed the wrong PR, but I think I had the same comments about the logging here:
Heh, yeah, the other one has this one's commits cherry-picked :) |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, the CI failures we can ignore: InstallAndroidDependenciesTest("GoogleV2")
(working to fix in other PRs)
And this one: GuavaListenableFuture: System.Net.WebException : The operation has timed out.
Logging to logcat in Android is expensive and unpredictable in
terms of how much time it takes exactly. Therefore, we try to
log as little as possible by default, reducing the delays.
This PR removes (or demotes to a lower priority) a number of log
messages that were useful during testing and development, but are
an unnecessary noise for day-to-day app operation.