search

sosedoff / xml-sitemap

Easy XML sitemap generation for Ruby/Rails/Merb/Sinatra applications
MIT License
57 stars 19 forks source link

Alternatives to using Builder for performance reasons #7

Closed danhealy closed 13 years ago

danhealy commented 13 years ago

I have another performance-related suggestion...

I benchmarked Builder's performance against Nokogiri and raw string concatenation. For ~50k entries: user system total real render 17.680000 0.050000 17.730000 ( 17.726192) render(:nokogiri) 8.180000 0.140000 8.320000 ( 8.327214) render(:string) 1.560000 0.030000 1.590000 ( 1.580630)

I implemented them as options to the render method:

render(:nokogiri) will use nokogiri render(:string) will use raw string concatenation

Let me know if this sounds like a good patch or if you have any other ideas (like just replacing Builder). It's on the performance branch on my fork:

https://github.com/danhealy/xml-sitemap/tree/performance

grempe-thrillcall commented 13 years ago

Having these performance options is great. For the Nokogiri option I might suggest not including it by default in the gemspec, and wrapping the "require 'nokogiri'" in a begin/rescue block that rescues LoadError so that the require statement will work whether or not nokogiri is loaded on a particular system. Then within the :nokogiri rendering option code I would test for the presence of a constant that is part of the Nokogiri code and allow use of nokogiri if it is detected.

The string method, if substantially faster than both of those, should probably become the default option.

sosedoff commented 13 years ago

Hey,

Im not really concerned about performance side of this gem since i've never used it for anything bigger that 500k records across multiple sitemaps, and even though generation would only happen once a week.

Originally this gem had no runtime dependencies, sitemap generation was all based on string (as you suggested).

I dont mind having different renderers, but would prefer having as less gem deps as possible. Maybe we should do something similar to what multi_json gem does. Detect xml engines on the fly, and if nothing is found - fallback to string renderer.

Let me know what do you think about that

danhealy commented 13 years ago

Great ideas, I removed the runtime dependency and defaulted render to use string.

https://github.com/danhealy/xml-sitemap/commit/2ab187b2304095f65d796fd091c35d88d252ed6e

This is branched off my earlier pull request.

sosedoff commented 13 years ago

Alright, looks good.

Will merge and work on new version tonight.

sosedoff commented 13 years ago

Would you mind opening the pull request for this feature?

sosedoff commented 13 years ago

shipped with 1.2.0