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 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 Aug 17, 2017
Jordon Bedwell and others added 4 commits Aug 12, 2017
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>
@parkr parkr force-pushed the envygeeks-allow-yield branch from f2d0a06 to 9737908 Aug 18, 2017
@DirtyF
DirtyF approved these changes Aug 18, 2017
Copy link
Member

DirtyF left a comment

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

@DirtyF

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

parkr commented Aug 18, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 232ec46 into master Aug 18, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot deleted the envygeeks-allow-yield branch Aug 18, 2017
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
4 participants
You can’t perform that action at this time.