Glob scope path only if configured with a pattern #6692

ashmaroli · 2018-01-11T06:51:04Z

Resolves #6691

Take the less expensive route unless the user configures the scope["path"] with a glob pattern. e.g. include

"content/*.pdf"
"content/**/*"

Crunch09

👍👏 LGTM! Might be worth adding a test to check that Dir.glob is no longer called for pattern-less paths?
Also can you confirm that this speeds up https://github.com/mmistakes/front-matter-defaults.git again?

DirtyF · 2018-01-11T13:02:44Z

On latest macOS with my macbook air and @mmistakes test repo:

3.6.2	3.7.0	Fix
16.279s	25.321s	15.913s
18.276s	30.145s	15.979s
17.406s	29.49s	16.09s

ashmaroli · 2018-01-11T13:26:59Z

Might be worth adding a test to check that

@Crunch09 That's a good idea to be extra sure.. Will see if I can come up with a simple check..

On latest macOS with my macbook air

@DirtyF Thank you very much for running the tests 💯

mmistakes · 2018-01-11T19:28:32Z

Tested the fix on the front-matter-defaults repo and my large personal site... both are back to building at 3.6.2 speeds.

😄

parkr · 2018-01-11T20:11:58Z

lib/jekyll/frontmatter_defaults.rb

@@ -123,6 +128,13 @@ def path_is_subpath?(path, parent_path)
      false
    end

+    def glob_pattern?(path)
+      path.each_filename do |str|
+        return true if str =~ %r!\*+|\?|\[.*\]|{.*}!


Does path.include?(“*”) not work for this case?

I wanted to test the path component string for specific glob patterns. and regex seems better suited for it than multiple String#include? calls

@ashmaroli Why do we need to test each filename separately? AFAIK, the presence of * in scope["path"] is enough to know it's a glob pattern.

@parkr

defaults: - scope: path: "_puppies/arrears.?" type: "puppies" values: layout: arrears

is also a glob pattern to catch all kinds of files with basename arrears, if you ask me..

I haven't seen or used ? before. Is that a Windows-ism?

Let's decide which glob patterns we should support and write individual tests using them so we know they work and are supported. The original PR only mentioned * which is why I mention that one only.

FWIW, for your example, I would have used "_puppets/arrears.*".

parkr · 2018-01-14T16:08:16Z

lib/jekyll/frontmatter_defaults.rb

-        scope_path = Pathname.new(scope_path).relative_path_from site_path
-        return true if path_is_subpath?(sanitized_path, scope_path)
+
+      if glob_pattern?(rel_scope_path)


So instead of this, we do if scope["path"].to_s.include?("*"). Or you can change the implementation of glob_pattern? to just do path.to_s.include?("*"). What do you think?

parkr

A simple unit test for glob_pattern? would be great if you don't mind. Using my suggestion:

should "match globs" do
  assert @site.frontmatter_defaults.glob_include?("content/*.pdf")
end

should "not match non-globs" do
  refute @site.frontmatter_defaults.glob_include?("layouts/")
end

ashmaroli · 2018-01-14T16:20:16Z

if you don't mind

@parkr I don't mind adding tests at all.. but just that I find it difficult to digest your suggestion above..
The absence of * in the scope["path"] doesn't mean it's not a glob pattern..

/cc @pathawks for your inputs.. (Thanks in advance..)

Regarding the unit test,
I won't be able to assert @site.frontmatter_defaults.glob_include?("content/*.pdf") because glob_include? is a private method.. ( yeah, I can use :send.. )

DirtyF · 2018-01-14T17:28:36Z

Those use case seems already fine for most users, we never spoke of supporting all possible regexp patterns in path, did we?

"content/*.pdf"
"content/**/*"

Let's try to keep it simple here for now, and ship a quick fix.

pathawks · 2018-01-17T22:28:48Z

lib/jekyll/frontmatter_defaults.rb

+          scope_path = Pathname.new(scope_path).relative_path_from site_path
+          Jekyll.logger.debug "Globbed Scope Path:", scope_path
+          return true if path_is_subpath?(sanitized_path, scope_path)
+        end
      end

      path_is_subpath?(sanitized_path, rel_scope_path)


Could we make this an else for the previous block (if scope["path"].to_s.include?("*"))?
Multiple exits can get confusing.

~~We can't make this an else branch because if diff-ln#113 evaluates to false even at the end of looping the globbed array, diff-ln#117 acts as a fallback..~~

~~That said, it does not make sense to me why that fallback's needed..~~

parkr

Looks good to me!

parkr · 2018-01-18T14:27:48Z

docs/_docs/configuration.md

+<div class="note warning">
+  <h5>Globbing and Performance</h5>
+  <p>
+    Please note that globbing a path is a known performance-poison and is


The poison sounds really terrifying! Can we rephrase?

Please note that front matter defaults with glob paths have known performance problems.
Windows users may notice especially high build times by adding globbing. We are tracking [in this issue](let's open an issue).

We are tracking [in this issue](let's open an issue).

I believe this is an issue with Ruby for Windows itself as Ruby is not optimized for Windows.. There's not much one can do here IMO..

So.. I'll just re-phrase the "poison" bit to something more mellow..

pathawks

👏🏼

DirtyF · 2018-01-19T19:26:47Z

@jekyllbot: merge +bug

ashmaroli · 2018-01-23T15:54:12Z

@mmistakes If possible can you please provide the following feedback on this change using your front-matter-defaults repo.. and Jekyll's master branch..?

Build times on Windows and Unix with jekyll build
Build times on Windows and Unix with jekyll build --watch followed by a minor change to about.md
Build times on Windows and Unix with jekyll serve followed by a minor change to about.md

Thanks in advance..

mmistakes · 2018-01-23T16:23:22Z

Sure thing @ashmaroli, here's what I'm getting on Windows.

	3.6.2	3.7.0	jekyll/jekyll `master`
1.	20.098s	48.487s	20.729s
2.	20.721s	49.879s	21.192s
3.	20.977s	50.313s	21.518s

ashmaroli · 2018-01-23T16:30:20Z

here's what I'm getting

Right.. and the regeneration times..?

mmistakes · 2018-01-23T16:32:56Z

Those are the regeneration times. It wasn't any faster regenerating the page after about.md was modified. Still took ~20s... or in the case of 3.7.0 around 50.

ashmaroli · 2018-01-23T16:37:39Z

Okay buddy.. Thanks a lot..
On my system (Windows 7, Ruby 2.3, jekyll:master and minima:master)
the build times were:

initial-build: 45.412 seconds
regeneratn.: 254.069825 seconds

Something's wrong somewhere!!

mmistakes · 2018-01-23T16:51:00Z

Forgot to mention, I added the wdm gem to my Gemfile as instructed by this warning:

gem 'wdm', '>= 0.1.0' if Gem.win_platform?

Without it the regen speeds were much slower for some reason.

Without `wdm`:

initial-build: 20.914 seconds
regeneration: 37.999 seconds

With `wdm`:

initial-build: 20.624 seconds
regeneration: 21.493 seconds

ashmaroli · 2018-01-23T17:23:13Z

MIND=BLOWN !!
Two years of Jekyll on Windows and I finally realize the truth behind the suggestion to include gem 'wdm'..

Without `wdm`:

initial-build: 47.503 seconds
regeneratn.: 246.402432 seconds

With `wdm`:

initial-build: 45.334 seconds
regeneratn.: 48.469285 seconds

💯 👏 @mmistakes

mmistakes · 2018-01-23T17:25:59Z

@ashmaroli 😃

I've never really known what adding wdm did. I feel like at one point --watch stopped working for me reliably so I added it, but didn't know it helped performance until now.

Glob scope path only if configured with a pattern

3ce57cd

ashmaroli requested a review from a team January 11, 2018 06:52

ashmaroli added the fix label Jan 11, 2018

ashmaroli added this to the v3.7.1 milestone Jan 11, 2018

Crunch09 approved these changes Jan 11, 2018

View reviewed changes

test if a scope path is not globbed unnecessarily

28559b7

parkr reviewed Jan 11, 2018

View reviewed changes

parkr reviewed Jan 14, 2018

View reviewed changes

parkr requested changes Jan 14, 2018

View reviewed changes

ashmaroli requested a review from pathawks January 14, 2018 16:20

ashmaroli mentioned this pull request Jan 15, 2018

Release v3.7.1 #6695

Merged

7 tasks

ashmaroli added 2 commits January 15, 2018 16:24

limit scope path globbing to just patterns with *

7764331

update docs on globbing in Front Matter defaults

d409d4a

pathawks reviewed Jan 17, 2018

View reviewed changes

add an else branch for better readability

a69ffba

parkr approved these changes Jan 18, 2018

View reviewed changes

pathawks approved these changes Jan 18, 2018

View reviewed changes

rephrase note in documentation

5b90b7e

jekyllbot merged commit 009d308 into jekyll:master Jan 19, 2018

jekyllbot added bug fix labels Jan 19, 2018

jekyllbot added a commit that referenced this pull request Jan 19, 2018

Update history to reflect merge of #6692 [ci skip]

6b98a5c

ashmaroli deleted the glob-opt-in branch January 24, 2018 14:38

ashmaroli mentioned this pull request Jan 24, 2018

Add gem "wdm" to all newly generated Gemfiles #6711

Merged

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glob scope path only if configured with a pattern #6692

Glob scope path only if configured with a pattern #6692

ashmaroli commented Jan 11, 2018 •

edited by DirtyF

Crunch09 left a comment

DirtyF commented Jan 11, 2018 •

edited

ashmaroli commented Jan 11, 2018

mmistakes commented Jan 11, 2018

parkr Jan 11, 2018

ashmaroli Jan 12, 2018

parkr Jan 14, 2018

ashmaroli Jan 14, 2018

parkr Jan 14, 2018

parkr Jan 14, 2018

parkr Jan 14, 2018

parkr left a comment •

edited

ashmaroli commented Jan 14, 2018

DirtyF commented Jan 14, 2018 •

edited

pathawks Jan 17, 2018

ashmaroli Jan 18, 2018 •

edited

parkr left a comment

parkr Jan 18, 2018

ashmaroli Jan 18, 2018

pathawks left a comment

DirtyF commented Jan 19, 2018

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018 •

edited

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018 •

edited

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018

Glob scope path only if configured with a pattern #6692

Glob scope path only if configured with a pattern #6692

Conversation

ashmaroli commented Jan 11, 2018 • edited by DirtyF

Crunch09 left a comment

Choose a reason for hiding this comment

DirtyF commented Jan 11, 2018 • edited

ashmaroli commented Jan 11, 2018

mmistakes commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr left a comment • edited

Choose a reason for hiding this comment

ashmaroli commented Jan 14, 2018

DirtyF commented Jan 14, 2018 • edited

Choose a reason for hiding this comment

ashmaroli Jan 18, 2018 • edited

Choose a reason for hiding this comment

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathawks left a comment

Choose a reason for hiding this comment

DirtyF commented Jan 19, 2018

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018 • edited

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018

ashmaroli commented Jan 23, 2018

mmistakes commented Jan 23, 2018 • edited

Without wdm:

With wdm:

ashmaroli commented Jan 23, 2018

Without wdm:

With wdm:

mmistakes commented Jan 23, 2018

ashmaroli commented Jan 11, 2018 •

edited by DirtyF

DirtyF commented Jan 11, 2018 •

edited

parkr left a comment •

edited

DirtyF commented Jan 14, 2018 •

edited

ashmaroli Jan 18, 2018 •

edited

mmistakes commented Jan 23, 2018 •

edited

mmistakes commented Jan 23, 2018 •

edited

Without `wdm`:

With `wdm`:

Without `wdm`:

With `wdm`: