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

Store and retrieve converter instances for Jekyll::Filters via a hash #6856

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Mar 18, 2018

Store converter instances into, and retrieve from a hash.
Since the associated filters may be called frequently within single Liquid template for various inputs, and the template itself may be called for a ton of documents / pages in a large site, its better to opt for hash-lookups over multiple Array#find

Summary:

  • Move to using multiple hash lookups against multiple Array#find
  • Existing local variables have been dropped since they serve no bigger purpose than to be created, used once and immediately garbage-collected away
  • @converter_instances remains defined for the duration of current Site instance since initialized converters don't change when the site regenerates
@pathawks

This comment has been minimized.

Member

pathawks commented Mar 20, 2018

Why not just change find_converter_instance to memoize? Does this really result in a big performance boost?

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 20, 2018

Why not just change find_converter_instance to memoize? Does this really result in a big performance boost?

We can't memoize by the traditional approach (using ||=) because the method takes a parameter and the Site class is already packed with instance_variables..
So, its better we take the alternative approach using a hash to cache and retrieve objects..

The performance boost is not much to write home about.. therefore it won't be perceivable to the end user..

@pathawks

This comment has been minimized.

Member

pathawks commented Mar 20, 2018

because the method takes a parameter

I must be missing something. Why not use the same hash table approach from converter_instance_of here, but in the find_converter_instance method instead? That way, any code that already uses find_converter_instance benefits.

therefore it won't be perceivable to the end user

So why is it worth the additional complexity?

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 20, 2018

Why not use the same hash table approach from converter_instance_of here, but in the find_converter_instance method instead? That way, any code that already uses find_converter_instance benefits

Ah.. I see it now..
Shot myself in the foot back there when i said Site is already loaded with instance_variables..

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 20, 2018

So why is it worth the additional complexity?

If a layout were to call markdownify filter 20 times and this layout was used by 50 documents,
converters.find { |klass_| klass_.instance_of?(klass) } would be executed 20*50 times..
Now if another layout in the same site had similar calls for smartify or scssify filters............

converters.find { |klass_| klass_.instance_of?(klass) } || \
@find_converter_instance ||= {}
@find_converter_instance[klass] = \
converters.find { |klass_| klass_.instance_of?(klass) } || \

This comment has been minimized.

@Crunch09

Crunch09 Mar 21, 2018

Member

I realise this didn't change in your PR but why is the block variable called klass_ instead of e.g. converter ? Seems a bit odd to me.

This comment has been minimized.

@ashmaroli

ashmaroli Mar 22, 2018

Member

Agreed. I can change it since it's part of the diff..

def find_converter_instance(klass)
converters.find { |klass_| klass_.instance_of?(klass) } || \
@find_converter_instance ||= {}
@find_converter_instance[klass] = \

This comment has been minimized.

@Crunch09

Crunch09 Mar 21, 2018

Member

I'm probably missing something but shouldn't this be @find_converter_instance[klass] ||= ?

This comment has been minimized.

@ashmaroli

ashmaroli Mar 22, 2018

Member

You're correct @Crunch09. Nice catch 👍

@DirtyF

DirtyF approved these changes Mar 22, 2018 edited

This PR shaves ~1s on Jekyll's website ⚡️

@ashmaroli ashmaroli added this to the v3.8.0 milestone Apr 9, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented Apr 12, 2018

@jekyllbot: 🚢 +minor

@jekyllbot jekyllbot merged commit 29dc190 into jekyll:master Apr 12, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:reduce-local-var-inits branch Apr 12, 2018

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