commonmarker icon indicating copy to clipboard operation
commonmarker copied to clipboard

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

Open movermeyer opened this issue 5 years ago • 1 comments

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)

movermeyer avatar Jan 03 '20 20:01 movermeyer

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 avatar Jan 05 '20 11:01 gjtorikian

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

gjtorikian avatar Nov 03 '22 19:11 gjtorikian