jekyll / jekyll-avatar

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

Parse only custom-host provided through ENV #45

Closed ashmaroli closed 4 years ago

ashmaroli commented 4 years ago

Rationale

Unless a non-empty string is set to environment variable PAGES_AVATARS_URL, the host attribute is "https://avatars#{server_number}.githubusercontent.com" which is already a valid URI (the interpolated server_number cannot be manipulated by end-user).

Therefore, it is wasteful to have Addressable::URI parse a valid URI on every render just to have another URI fragment string (which is never parsed in either case) joined to the above host.

Benefits

Since the process here involves just string manipulation, the operation is in-theory, faster and involves reduced memory consumption (since there is very minimal number of intermediary objects created).

ashmaroli commented 4 years ago

Thanks for the invite @benbalter :smile: . Is there anything I need to clarify about the changes here..?

ashmaroli commented 4 years ago

@benbalter If this is not going to be released, can I request a release for the commits already on master? Thanks.

benbalter commented 4 years ago

@ashmaroli sorry I missed this and thanks for this PR.

the operation is in-theory, faster and involves reduced memory consumption

Are you able to quickly benchmark this PR vs. master? My only concern is the added complexity it adds may not outweigh the potential performance benefit.

ashmaroli commented 4 years ago

@benbalter I see a 6x improvement (for avatars on default host) on my system (Ruby 2.4 on Windows) with the following script run with master and the PR branch:

# frozen_string_literal: true

require 'benchmark/ips'
require 'bundler/setup'
require 'jekyll'
require 'jekyll-avatar'

CONTENTS = <<~TEXT
  {% avatar benbalter %}
TEXT
TEMPLATE = Liquid::Template.parse(CONTENTS, line_numbers: true)

puts ""

Benchmark.ips do |x|
  x.report('master') { TEMPLATE.render }
  x.report('pull-request') { TEMPLATE.render }
  x.hold! File.expand_path('bench-temp')
  x.compare!
end

To test:

benbalter commented 4 years ago

Thanks, once again @ashmaroli!