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.

Sign up for GitHub

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

Jump to bottom

Compute relative_path of pages using PathManager #8408

Merged
merged 7 commits into from
Sep 30, 2020
Merged

Compute relative_path of pages using PathManager #8408

merged 7 commits into from
Sep 30, 2020

Conversation

ashmaroli
Copy link
Member

  • This is a 馃敤 code refactoring change.
  • I've added tests.

Summary

File.join is sensitive to nil arguments. So we need to coerce arguments to String to be safer.
Using Jekyll::PathManager.join on the other hand allows optimized execution for same results.

Context

Prior discussions: #8404 (comment)

@ashmaroli
Copy link
Member Author

Benchmark report

Results from Ruby 2.4 on Windows

Warming up --------------------------------------
       nil_page slow    23.239k i/100ms
       nil_page fast    17.135k i/100ms
Calculating -------------------------------------
       nil_page slow    250.293k (A卤16.1%) i/s -      1.232M in   5.016589s
       nil_page fast    166.101k (A卤 5.3%) i/s -    839.615k in   5.067538s

Comparison:
       nil_page slow:   250293.3 i/s
       nil_page fast:   166101.0 i/s - 1.51x  (A卤 0.00) slower

Warming up --------------------------------------
     empty_page slow    26.692k i/100ms
     empty_page fast    16.557k i/100ms
Calculating -------------------------------------
     empty_page slow    295.685k (A卤 5.0%) i/s -      1.495M in   5.067854s
     empty_page fast    183.103k (A卤 5.0%) i/s -    927.192k in   5.075815s

Comparison:
     empty_page slow:   295684.9 i/s
     empty_page fast:   183102.7 i/s - 1.61x  (A卤 0.00) slower

Warming up --------------------------------------
      root_page slow    18.165k i/100ms
      root_page fast    14.024k i/100ms
Calculating -------------------------------------
      root_page slow    191.299k (A卤11.2%) i/s -    962.745k in   5.081186s
      root_page fast    184.917k (A卤 5.6%) i/s -    925.584k in   5.022202s

Comparison:
      root_page slow:   191299.4 i/s
      root_page fast:   184916.8 i/s - same-ish: difference falls within error

Warming up --------------------------------------
    nested_page slow    16.210k i/100ms
    nested_page fast    29.264k i/100ms
Calculating -------------------------------------
    nested_page slow    159.222k (A卤 5.1%) i/s -    810.500k in   5.103311s
    nested_page fast    297.392k (A卤 6.9%) i/s -      1.492M in   5.042992s

Comparison:
    nested_page fast:   297392.0 i/s
    nested_page slow:   159222.4 i/s - 1.87x  (A卤 0.00) slower

Warming up --------------------------------------
   slashed_page slow    12.482k i/100ms
   slashed_page fast    18.147k i/100ms
Calculating -------------------------------------
   slashed_page slow    122.108k (A卤 4.6%) i/s -    611.618k in   5.019493s
   slashed_page fast    189.240k (A卤12.1%) i/s -    943.644k in   5.046522s

Comparison:
   slashed_page fast:   189239.8 i/s
   slashed_page slow:   122107.6 i/s - 1.55x  (A卤 0.00) slower

The proposed change is only slow for edge-cases. However, there is significant improvement for pages in subdirectories.

@ashmaroli ashmaroli mentioned this pull request Sep 24, 2020
Merged
@ashmaroli ashmaroli added this to the 4.2 milestone Sep 24, 2020
@ashmaroli ashmaroli mentioned this pull request Sep 27, 2020
Merged
DirtyF
DirtyF approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@DirtyF DirtyF left a comment •
edited

Choose a reason for hiding this comment

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

Looks good now.

parkr
parkr approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Nice! One question.

test/test_page_without_a_file.rb
page = PageWithoutAFile.new(@site, @site.source, dir, name)
assert_equal result, page.path
assert_equal result, page.relative_path
refute page.relative_path.frozen?
Copy link
Member

Choose a reason for hiding this comment

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

Is the relative path modified? Why is it important that it's unfrozen?

Copy link
Member Author

@ashmaroli ashmaroli Sep 28, 2020 •
edited

Choose a reason for hiding this comment

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

For backwards-compatibility. It is an unfrozen string in v4.1.
Jekyll::PathManager.join returns a frozen cached string. So this test confirms that page.relative_path hasn't changed behavior in v4.2..

@ashmaroli
Copy link
Member Author

Thank you the reviews @DirtyF and @parkr
@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 7cb10df into jekyll:master Sep 30, 2020
jekyllbot added a commit that referenced this pull request Sep 30, 2020
@jekyllbot
Update history to reflect merge of #8408 [ci skip]
166796c
@ashmaroli ashmaroli deleted the page-relative-path branch September 30, 2020 06:11
This was referenced Mar 12, 2021
Closed
Merged
Merged
Merged
Merged
@jekyll jekyll locked and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@parkr parkr parkr approved these changes

@DirtyF DirtyF Awaiting requested review from DirtyF

Assignees
No one assigned
Labels
bug fix frozen-due-to-age
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ashmaroli @DirtyF @parkr @jekyllbot