8000 Optimize `Jekyll::Utils.parse_date` by fauno · Pull Request #8425 · jekyll/jekyll · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Optimize Jekyll::Utils.parse_date #8425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 22, 2021
Merged

Optimize Jekyll::Utils.parse_date #8425

merged 4 commits into from
Jul 22, 2021

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 fauno force-pushed the cache-time-parse branch from f7b593c to 159b66c Compare October 5, 2020 17:35
@fauno
Copy link
Member Author
fauno commented Oct 5, 2020 •
edited
Loading

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
DirtyF
DirtyF approved these changes Nov 11, 2020
View reviewed changes
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 :)

@fauno fauno mentioned this pull request Jan 2, 2021
Open
@DirtyF DirtyF requested a review from a team January 13, 2021 07:32
@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
@jekyllbot
Update history to reflect merge of #8425 [ci skip]
ea535a9
github-actions bot pushed a commit that referenced this pull request Jul 22, 2021
@jekyllbot
Deploy docs from 0dee662
2cd1a9a 
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.
Reviewers

@DirtyF DirtyF DirtyF approved these changes

Assignees
No one assigned
Labels
enhancement frozen-due-to-age optimization ⚡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fauno @ashmaroli @DirtyF @jekyllbot
0