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

Page#dir: ensure it ends in a slash #4403

Merged
merged 4 commits into from Jan 27, 2016

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Jan 27, 2016

Fixes #3974.

/cc @envygeeks, because I'm sure you have a faster/more elegant/more performant solution for this problem.
/cc @jekyll/core for review!

@parkr parkr added the fix label Jan 27, 2016

@parkr parkr added this to the 3.1.1 milestone Jan 27, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 27, 2016

Contributor
def _new(url)
  return url if url.end_with?("/") || url == "/"

  url = File.dirname(url)
  url == "/" ? url : "#{url}/"
end
Calculating -------------------------------------
              _old:/    60.796k i/100ms
              _new:/    72.437k i/100ms
-------------------------------------------------
              _old:/      2.489M (± 4.8%) i/s -     12.463M
              _new:/      4.657M (± 8.5%) i/s -     23.107M

Comparison:
              _new:/:  4657430.1 i/s
              _old:/:  2489307.2 i/s - 1.87x slower

Calculating -------------------------------------
   _old:/hello/world    43.174k i/100ms
   _new:/hello/world    46.844k i/100ms
-------------------------------------------------
   _old:/hello/world      1.172M (± 3.5%) i/s -      5.872M
   _new:/hello/world      1.261M (± 4.0%) i/s -      6.324M

Comparison:
   _new:/hello/world:  1261134.4 i/s
   _old:/hello/world:  1172086.2 i/s - 1.08x slower

Calculating -------------------------------------
  _old:/hello/world/    61.655k i/100ms
  _new:/hello/world/    72.840k i/100ms
-------------------------------------------------
  _old:/hello/world/      2.531M (± 3.4%) i/s -     12.639M
  _new:/hello/world/      4.752M (± 4.0%) i/s -     23.746M

Comparison:
  _new:/hello/world/:  4751972.9 i/s
  _old:/hello/world/:  2531102.8 i/s - 1.88x slower


You can edge out more performance with url + "/" but that's a violation of Github's styleguide which I follow so I did not include it in the results.

Contributor

envygeeks commented Jan 27, 2016

def _new(url)
  return url if url.end_with?("/") || url == "/"

  url = File.dirname(url)
  url == "/" ? url : "#{url}/"
end
Calculating -------------------------------------
              _old:/    60.796k i/100ms
              _new:/    72.437k i/100ms
-------------------------------------------------
              _old:/      2.489M (± 4.8%) i/s -     12.463M
              _new:/      4.657M (± 8.5%) i/s -     23.107M

Comparison:
              _new:/:  4657430.1 i/s
              _old:/:  2489307.2 i/s - 1.87x slower

Calculating -------------------------------------
   _old:/hello/world    43.174k i/100ms
   _new:/hello/world    46.844k i/100ms
-------------------------------------------------
   _old:/hello/world      1.172M (± 3.5%) i/s -      5.872M
   _new:/hello/world      1.261M (± 4.0%) i/s -      6.324M

Comparison:
   _new:/hello/world:  1261134.4 i/s
   _old:/hello/world:  1172086.2 i/s - 1.08x slower

Calculating -------------------------------------
  _old:/hello/world/    61.655k i/100ms
  _new:/hello/world/    72.840k i/100ms
-------------------------------------------------
  _old:/hello/world/      2.531M (± 3.4%) i/s -     12.639M
  _new:/hello/world/      4.752M (± 4.0%) i/s -     23.746M

Comparison:
  _new:/hello/world/:  4751972.9 i/s
  _old:/hello/world/:  2531102.8 i/s - 1.88x slower


You can edge out more performance with url + "/" but that's a violation of Github's styleguide which I follow so I did not include it in the results.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 27, 2016

Member

👍 for @envygeeks' function. Though I think the || url == "/" can be removed from the first line? (Since .end_with? "/" should return true in this case)

Member

alfredxing commented Jan 27, 2016

👍 for @envygeeks' function. Though I think the || url == "/" can be removed from the first line? (Since .end_with? "/" should return true in this case)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 27, 2016

Contributor

@alfredxing it could be, but it's slower.

Contributor

envygeeks commented Jan 27, 2016

@alfredxing it could be, but it's slower.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 27, 2016

Member

OK! But (just a sanity check) doesn't the first part (url.end_with?("/")) get evaluated first, then url == "/"?

Member

alfredxing commented Jan 27, 2016

OK! But (just a sanity check) doesn't the first part (url.end_with?("/")) get evaluated first, then url == "/"?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 27, 2016

Contributor

It should but sadly the code ran faster with a more direct check, in there too. I'll double check the benches again just to make sure, because you should and probably are right.

Contributor

envygeeks commented Jan 27, 2016

It should but sadly the code ran faster with a more direct check, in there too. I'll double check the benches again just to make sure, because you should and probably are right.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 27, 2016

Member

Ok, I just pushed up another iteration. A bit longer, but doing every little thing helped:

Calculating -------------------------------------
            pre_pr:/   151.086k i/100ms
                pr:/   168.945k i/100ms
         envygeeks:/   168.146k i/100ms
-------------------------------------------------
            pre_pr:/      5.652M (± 4.5%) i/s -     28.253M
                pr:/      8.017M (± 5.4%) i/s -     40.040M
         envygeeks:/      7.445M (±10.1%) i/s -     36.824M

Comparison:
                pr:/:  8017279.2 i/s
         envygeeks:/:  7444605.1 i/s - 1.08x slower
            pre_pr:/:  5651646.7 i/s - 1.42x slower

Calculating -------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like
                        87.137k i/100ms
pr:/some/very/very/long/path/to/a/file/i/like
                        62.901k i/100ms
envygeeks:/some/very/very/long/path/to/a/file/i/like
                        60.416k i/100ms
-------------------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like
                          1.396M (± 6.4%) i/s -      6.971M
pr:/some/very/very/long/path/to/a/file/i/like
                        895.033k (± 5.4%) i/s -      4.466M
envygeeks:/some/very/very/long/path/to/a/file/i/like
                        891.898k (± 4.9%) i/s -      4.471M

Comparison:
pre_pr:/some/very/very/long/path/to/a/file/i/like:  1396111.2 i/s
pr:/some/very/very/long/path/to/a/file/i/like:   895033.2 i/s - 1.56x slower
envygeeks:/some/very/very/long/path/to/a/file/i/like:   891897.8 i/s - 1.57x slower

Calculating -------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like/
                       150.546k i/100ms
pr:/some/very/very/long/path/to/a/file/i/like/
                       165.396k i/100ms
envygeeks:/some/very/very/long/path/to/a/file/i/like/
                       162.486k i/100ms
-------------------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like/
                          5.570M (± 5.8%) i/s -     27.851M
pr:/some/very/very/long/path/to/a/file/i/like/
                          7.821M (± 7.1%) i/s -     39.033M
envygeeks:/some/very/very/long/path/to/a/file/i/like/
                          7.734M (± 7.6%) i/s -     38.509M

Comparison:
pr:/some/very/very/long/path/to/a/file/i/like/:  7821419.0 i/s
envygeeks:/some/very/very/long/path/to/a/file/i/like/:  7733559.1 i/s - 1.01x slower
pre_pr:/some/very/very/long/path/to/a/file/i/like/:  5569945.8 i/s - 1.40x slower
Member

parkr commented Jan 27, 2016

Ok, I just pushed up another iteration. A bit longer, but doing every little thing helped:

Calculating -------------------------------------
            pre_pr:/   151.086k i/100ms
                pr:/   168.945k i/100ms
         envygeeks:/   168.146k i/100ms
-------------------------------------------------
            pre_pr:/      5.652M (± 4.5%) i/s -     28.253M
                pr:/      8.017M (± 5.4%) i/s -     40.040M
         envygeeks:/      7.445M (±10.1%) i/s -     36.824M

Comparison:
                pr:/:  8017279.2 i/s
         envygeeks:/:  7444605.1 i/s - 1.08x slower
            pre_pr:/:  5651646.7 i/s - 1.42x slower

Calculating -------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like
                        87.137k i/100ms
pr:/some/very/very/long/path/to/a/file/i/like
                        62.901k i/100ms
envygeeks:/some/very/very/long/path/to/a/file/i/like
                        60.416k i/100ms
-------------------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like
                          1.396M (± 6.4%) i/s -      6.971M
pr:/some/very/very/long/path/to/a/file/i/like
                        895.033k (± 5.4%) i/s -      4.466M
envygeeks:/some/very/very/long/path/to/a/file/i/like
                        891.898k (± 4.9%) i/s -      4.471M

Comparison:
pre_pr:/some/very/very/long/path/to/a/file/i/like:  1396111.2 i/s
pr:/some/very/very/long/path/to/a/file/i/like:   895033.2 i/s - 1.56x slower
envygeeks:/some/very/very/long/path/to/a/file/i/like:   891897.8 i/s - 1.57x slower

Calculating -------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like/
                       150.546k i/100ms
pr:/some/very/very/long/path/to/a/file/i/like/
                       165.396k i/100ms
envygeeks:/some/very/very/long/path/to/a/file/i/like/
                       162.486k i/100ms
-------------------------------------------------
pre_pr:/some/very/very/long/path/to/a/file/i/like/
                          5.570M (± 5.8%) i/s -     27.851M
pr:/some/very/very/long/path/to/a/file/i/like/
                          7.821M (± 7.1%) i/s -     39.033M
envygeeks:/some/very/very/long/path/to/a/file/i/like/
                          7.734M (± 7.6%) i/s -     38.509M

Comparison:
pr:/some/very/very/long/path/to/a/file/i/like/:  7821419.0 i/s
envygeeks:/some/very/very/long/path/to/a/file/i/like/:  7733559.1 i/s - 1.01x slower
pre_pr:/some/very/very/long/path/to/a/file/i/like/:  5569945.8 i/s - 1.40x slower
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 27, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jan 27, 2016

@jekyllbot: merge +bug

jekyllbot added a commit that referenced this pull request Jan 27, 2016

@jekyllbot jekyllbot merged commit 6cd393c into master Jan 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the page.dir-must-have-slash branch Jan 27, 2016

jekyllbot added a commit that referenced this pull request Jan 27, 2016

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.