search

jekyll / jekyll-avatar

A Jekyll plugin for rendering GitHub avatars
MIT License
89 stars 9 forks source link

Wasteful allocations due to multiple parsing of the same `host` url #35

Closed ashmaroli closed 5 years ago

ashmaroli commented 5 years ago

Summary

Multiple parsing of the same host url is a waste of resources. Based on the following source-code, #url is called at least 4 times per call to #render. Each call to #url however calls Addressable::URI.parse host to parse the same host url string (depends on an environment variable value) https://github.com/benbalter/jekyll-avatar/blob/9d6fb27c5bdfee1fedcbd871a1f643047b87fc3d/lib/jekyll-avatar.rb#L74-L91

I tried extracting Addressable::URI.parse host into a memoized private method. While benchmarks show that it's faster, the test suite fails..

What would be the best solution for this..?

benbalter commented 5 years ago

@ashmaroli since host is a private method, can host return a (memoized) URI (and update the tests to call to_s as necessary)?

ashmaroli commented 5 years ago

@benbalter The reason tests fail is because the suite tests both ENV scenarios in the same process. Memoization causes the update to env var to be ignored..

benbalter commented 5 years ago

Ahh, can we set the memoization to nil between each tests then?

ashmaroli commented 5 years ago

can we set the memoization to nil between each tests then?

That should be doable via RSpec's :before blocks. :+1: But I think your first suggestion to return Addressable::URI object has merit as well.. I'll open a PR with that and proceed.. would that be fine..?

ashmaroli commented 5 years ago

I had to drop memoization in favor of caching to maintain expected results.. Details in the PR :point_up_2:

ashmaroli commented 5 years ago

Thanks for the input @benbalter