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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Jekyll::Utils.parse_date #8425

Merged
merged 4 commits into from Jul 22, 2021
Merged

Conversation

fauno
Copy link
Member

@fauno fauno commented Oct 5, 2020

This is a 馃敤 code refactoring.

Summary

Same as #8424, let me know if I should write a benchmark :)

@DirtyF DirtyF requested a review from a team October 5, 2020 15:20
@ashmaroli
Copy link
Member

Similar to #8424, Jekyll::Utils is a module as well.
Instance variables should be used carefully since modules can be mixed-in into various classes.

Would like to see your profiling / benchmark results. Is Time.parse(input) slower or Time.parse(input).localtime ?
If it is just Time.parse(input), then we could just cache the result of Time.parse(input) and call :localtime on it to get a (mutable) non-cached string result.

@fauno
Copy link
Member Author

fauno commented Oct 5, 2020

It seems to be Time.parse rather than :localtime:

Warming up --------------------------------------
          Time.parse     1.439k i/100ms
           localtime     1.427k i/100ms
    localtime parsed   481.587k i/100ms
    Utils.parse_date   201.939k i/100ms
Calculating -------------------------------------
          Time.parse     14.465k (卤 1.7%) i/s -     73.389k in   5.075361s
           localtime     14.221k (卤 2.0%) i/s -     71.350k in   5.019481s
    localtime parsed      4.798M (卤 1.3%) i/s -     24.079M in   5.019862s
    Utils.parse_date      1.982M (卤 1.6%) i/s -     10.097M in   5.096938s

@ashmaroli ashmaroli changed the title Cache time parsing Optimize Jekyll::Utils.parse_date Oct 7, 2020
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.

LGTM

@DirtyF DirtyF added this to the 4.2 milestone Nov 11, 2020
@ashmaroli
Copy link
Member

@fauno I just came across an observation that calling :localtime on a Time object actually mutates the given Time object. Can you confirm..?

@ashmaroli ashmaroli removed this from the 4.2 milestone Nov 20, 2020
@ashmaroli ashmaroli marked this pull request as draft December 10, 2020 15:24
@fauno
Copy link
Member Author

fauno commented Dec 29, 2020

Sorry, I came back to see what happened with this PR and it seems I missed the email! I confirmed with documentation and a quick freeze test (Time.now.utc.freeze.localtime throws FrozenError) so I changed the PR a little. Should I rebase?

@ashmaroli
Copy link
Member

You need not rebase just yet, @fauno unless you'd like to use this branch in one of your projects.

@fauno fauno marked this pull request as ready for review December 29, 2020 18:59
@fauno
Copy link
Member Author

fauno commented Dec 29, 2020

Ok, everything seems fine, thanks for reviewing :)

@DirtyF
Copy link
Member

DirtyF commented Jul 22, 2021

@jekyll: merge +minor

@jekyllbot jekyllbot merged commit 0dee662 into jekyll:master Jul 22, 2021
jekyllbot added a commit that referenced this pull request Jul 22, 2021
github-actions bot pushed a commit that referenced this pull request Jul 22, 2021
fauno: Optimize `Jekyll::Utils.parse_date` (#8425)

Merge pull request 8425
@fauno fauno deleted the cache-time-parse branch July 22, 2021 18:12
@jekyll jekyll locked and limited conversation to collaborators Jul 22, 2022
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.

None yet

4 participants