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

Allow yield to logger methods & bail early on no-op messages #6315

Merged
merged 5 commits into from Aug 18, 2017

Conversation

parkr
Copy link
Member

@parkr parkr commented Aug 17, 2017

This subsumes #6306 and fixes the bugs with that PR.

It also bails early for debug, info, warn, and error if the logger's log level is higher than the message's log level. In this case the logging writer will skip it, so it's best to skip it here. It reduces the number of strings we create and greatly reduces the size of the messages array. This fixes #6313.

@parkr parkr requested review from pathawks, DirtyF and a user August 17, 2017 18:31
Jordon Bedwell and others added 4 commits August 17, 2017 22:20
Standard loggers in Ruby mostly `yield` their messages (this is the default behavior of Ruby's own logger.) Or if there is a block then the message is the topic, a colon is automatically added, and the message is shipped with the topic.  Jekyll should conform to this behavior so that it's logger can be passed to libraries that expect this standard behavior.

```ruby
[1] pry(main)> Jekyll.logger.info("Just so you know") # => Just so you know
[2] pry(main)> Jekyll.logger.info("Just so you know", "this is a message") # => Just so you know this is a message
[3] pry(main)> Jekyll.logger.info("Just so you know") { "this is a message" }
  # => Just so you know: this is a message
```

Signed-off-by: Parker Moore <parkrmoore@gmail.com>
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

I can only admire the work here, this seems pretty solid. 👏

@DirtyF
Copy link
Member

DirtyF commented Aug 18, 2017

Maybe @envygeeks wants to do a proper benchmark to see how this affect its build performance?

With Jekyll's script/test on my MacBook Air:

Before:

real	2m24.422s
user	1m56.664s
sys	0m11.537s

After:

real	2m6.318s
user	1m42.051s
sys	0m9.632s

@parkr
Copy link
Member Author

parkr commented Aug 18, 2017

Whoa, who knew logging could be so expensive! I just remembered that we can push this even further by setting the log level to :error by default in the tests.

On my MBP:

real	1m49.854s
user	1m28.382s
sys	0m8.263s

@parkr
Copy link
Member Author

parkr commented Aug 18, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 232ec46 into master Aug 18, 2017
@jekyllbot jekyllbot deleted the envygeeks-allow-yield branch August 18, 2017 16:45
jekyllbot added a commit that referenced this pull request Aug 18, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jekyll storing messages in Logger, it's not using, slowing down builds, +mem
4 participants