Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Be more aggressive when checking if a log message should be sent to the underlying logger #131

Closed
wants to merge 3 commits into from

Conversation

leonardehrenfried
Copy link
Contributor

@leonardehrenfried leonardehrenfried commented Nov 7, 2017

sbt/sbt#3711 has shown that substantial amount of time is spent calculating log messages that are unlikely to be displayed.

This PR introduces checks that filter out log events for disabled log levels.

It does this on two levels of abstraction which may be redundant and is deserving of critical review. The reason for this is there is non-trivial message construction is happening in ManagedLogger but to me changing AbstractLogger would offer a more general solution for all types of loggers.

Lastly, I upgraded the versions of scala as well as the scalafmt plugin.

@eed3si9n eed3si9n added the ready label Nov 7, 2017
As per sbt/sbt#3711 this was identified as a
performance hotspot.
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @leonardehrenfried.

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

This goes against the current design of sbt logging.
In sbt 1 the logger doesn't fully know whether something should be logged or not since that's determined by the appenders that subscribes to the logger. Making this change would break last command.

The log message produced by Ivy (sbt/sbt#3711) I think should be treated as special case since the integration of Ivy logging (via slf4j?) is something new to sbt 1.

@leonardehrenfried
Copy link
Contributor Author

leonardehrenfried commented Nov 8, 2017

@eed3si9n Thanks for the review. Do you think that changing https://github.com/sbt/librarymanagement/blob/1.x/ivy/src/main/scala/sbt/internal/librarymanagement/IvyLogger.scala#L25 would be the way forward?

Or should sbt/util be changed and check the logger name to filter out ivy log events?

@leonardehrenfried
Copy link
Contributor Author

Closed in favour of #132

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants