Skip to content
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

Request: Allow custom renderers to use C code for any methods they don't overwrite #112

Closed
movermeyer opened this issue Jan 3, 2020 · 2 comments
Labels

Comments

@movermeyer
Copy link

Let me start by saying that I'm not sure this is possible.


I need to add a custom attribute to all links. So I made a custom renderer:

class CustomCommonMarker < CommonMarker::HtmlRenderer
    attr_accessor :link_attributes

    def link(node)
      # Based on https://github.com/gjtorikian/commonmarker/blob/1fe234820a77cb3f2b26b47971d229d0da92d652/lib/commonmarker/renderer/html_renderer.rb#L130-L136
      attributes = { href: node.url.nil? ? '' : escape_href(node.url) }
      attributes[:title] = escape_html(node.title) if node.title.present?
      attributes.merge!(link_attributes || {})
      formatted_attrs = attributes.map { |k, v| "#{k}=\"#{v}\"" }.join(' ')
      out("<a #{formatted_attrs}>", :children, '</a>')
    end
  end

and I then use it:

custom_renderer = CustomCommonMarker.new(options: :UNSAFE, extensions: [:autolink])
custom_renderer.link_attributes = { class: 'body-link' }
parsed_doc = CommonMarker.render_doc(text, :UNSAFE, [:autolink])
custom_renderer.render(parsed_doc)

It works perfectly, but of course it is slower. How slow?

    Benchmark.ips do |x|
      x.time = 15
      x.report('commonmarker_render_html') do
        CommonMarker.render_html(text, :UNSAFE, [:autolink])
      end

      x.report('commonmarker_two_step') do
        parsed_doc = CommonMarker.render_doc(text, :UNSAFE, [:autolink])
        parsed_doc.to_html(:UNSAFE, [:autolink])
      end

      x.report('commonmarker_html_renderer') do
        custom_renderer = CommonMarker::HtmlRenderer.new(options: :UNSAFE, extensions: [:autolink])
        parsed_doc = CommonMarker.render_doc(text, :UNSAFE, [:autolink])
        custom_renderer.render(parsed_doc)
      end

      x.report('commonmarker_our_custom_renderer') do
        custom_renderer = CustomCommonMarker.new(options: :UNSAFE, extensions: [:autolink])
        custom_renderer.link_attributes = { class: 'body-link' }
        parsed_doc = CommonMarker.render_doc(text, :UNSAFE, [:autolink])
        custom_renderer.render(parsed_doc)
      end

      x.compare!
    end
Calculating -------------------------------------
commonmarker_render_html
                          1.824k (±14.0%) i/s -     26.680k in  15.013070s
commonmarker_two_step
                          1.133k (±30.4%) i/s -     13.152k in  15.027479s
commonmarker_html_renderer
                        202.258  (± 9.4%) i/s -      3.024k in  15.090556s
commonmarker_our_custom_renderer
                        198.886  (± 9.6%) i/s -      2.960k in  15.021675s

Comparison:
commonmarker_render_html:     1824.3 i/s
commonmarker_two_step:     1132.7 i/s - 1.61x  slower
commonmarker_html_renderer:      202.3 i/s - 9.02x  slower
commonmarker_our_custom_renderer:      198.9 i/s - 9.17x  slower

9x slower just to add an attribute on each link. 😢

It would be lovely if custom renderers could call the C code to render any node type that wasn't overridden. That way, the commonmarker_html_renderer would be able to perform similarly to commonmarker_two_step (1.6x slower instead of 8x)

@gjtorikian gjtorikian added the TODO label Jan 5, 2020
@gjtorikian
Copy link
Owner

Yes, I think that makes sense. I do believe the custom renderer right now is an "all or nothing" override. And I do believe it's possible in Ruby to some identification of overwritten child methods. I'll put this as a TODO for now.

PS: if Shopify is relying on Commonmarker, please consider sponsoring the project. Thanks. ✌️

@gjtorikian
Copy link
Owner

As of #184, all C code has been removed and this gem now uses a Rust backed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants