Skip to content

Avoid creating a closure for each log message #25922

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

Merged
merged 1 commit into from
Feb 16, 2018
Merged

Conversation

c42f
Copy link
Member

@c42f c42f commented Feb 7, 2018

Closures aren't strictly required for delayed log message creation, so we can remove them to avoid #15276 and reduce the load on the compiler.

This should fix #25909 (CC @maleadt).

[Edit: changed the description for clarity]

Delayed message creation via closures never ended up explicitly exposed
in the public logging API so we can remove the closures to avoid #15276.
@maleadt
Copy link
Member

maleadt commented Feb 7, 2018

Ah, too bad, delayed message was a valuable feature for me. Maybe a secondary API (@if_debug), or keyword option to eg. @debug, could offer similar functionality?

@JeffBezanson
Copy link
Member

Would it make sense to create a closure only if the message requires computation, i.e. isn't a constant string?

@ararslan ararslan added the logging The logging framework label Feb 7, 2018
@c42f
Copy link
Member Author

c42f commented Feb 7, 2018

There's a misunderstanding here (my fault for being unclear): this change doesen't modify the public logging API and message computation is still deferred until shouldlog(current_logger(), some_static_info) returns true.

The only thing which changes is an implementation detail:

  • Create a message generation closure and pass it to an internal function which later executes it, vs
  • Just insert the try/catch inline in the caller, along with the message generation code.

@c42f
Copy link
Member Author

c42f commented Feb 15, 2018

CI errors seem unrelated (I restarted the OSX build which had a weird failure in the spawn tests, probably wasn't worth it though).

If there's no objections to the newly cleaned up explanation I'll merge this in the next day or so.

@c42f c42f merged commit 6d8ee74 into master Feb 16, 2018
@martinholters martinholters deleted the cjf/no-logging-closures branch February 16, 2018 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging The logging framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logging macros generated code quality
4 participants