sparklemotion / nokogiri

Nokogiri (鋸) makes it easy and painless to work with XML and HTML from Ruby.
https://nokogiri.org/
MIT License
6.15k stars 901 forks source link

#xpath performance #760

Closed etm closed 4 months ago

etm commented 12 years ago

about 1/3 of the runtime of #xpath is spent allocating XPathContext. Not sure, but do you think caching the context could lead to bad situations? Caching would improve performance a lot.

Example (can be inserted into #xpath)

 @oldns ||= {}
 if @oldns == ns
   @ctx ||= XPathContext.new(self)
 else
   @ctx = XPathContext.new(self)
   @ctx.register_namespaces(ns)
  @oldns = ns
 end
...
@ctx.evaluate(....)

Note: adding and initializing @ctx and @oldns in the constructor is bad, as nodes that never need a ctx are created quite often, and thus this would degrade performance again.

cheers,

eTM

kivikakk commented 12 years ago

I thought that this would be a good idea, so I tried implementing your suggestion to see how it would work.

I'm seeing failures where nodes aren't being found as a result, so it seems that it's not good enough to simply cache the XPathContext like this, unfortunately.

etm commented 12 years ago

consider the following piece of code:

 require 'rubygems'
 require 'nokogiri'

 doc = Nokogiri.XML("<foo><baz/><baz><bar name='test'/></baz></foo>")
 xpa = Nokogiri::XML::XPathContext.new(doc.root)

 p xpa.evaluate('//baz').length # => 2
 p xpa.evaluate('baz').length # => 2
 p xpa.evaluate('//@name').length # => 1
 p xpa.evaluate('baz').length # => 0
 p xpa.evaluate('//baz').length # => 2
 p xpa.evaluate('foo').length # => 1 

These results are rather strange. As soon as you look for attributes somehow the context shifts. After the third xpath, we find foo instead of baz.

I think this is a bug in libxml - or some well designed behaviour i don't get :-). We could circumvent it maybe if we make the context node settable not only in the constructor for XPathContext, but afterwards (libxml has this too). According to my tests this is still MUCH faster. E.g.:

xpa = Nokogiri::XML::XPathContext.new(doc.root)
.... some xpath
xpa.node = doc.root
.... some other xpath

Juergen

kivikakk commented 12 years ago

I don't have the code in front of me to check, but it does seem probable to me that this is by design. (Otherwise it seems like an obvious optimisation that Nokogiri would have possibly already taken up.)

By the way, your example was difficult to follow; I think some XML was stripped. If you encase the code examples in triple-backticks it'll render it literally.

Next time I get a chance I'll see if I can find what's going on with this.

flavorjones commented 12 years ago

@etm - 1/3 of the time spent ... how much time is that? Can you show how you're benchmarking and tell us what Ruby version (output from nokogiri -v please)?

kivikakk commented 12 years ago

@flavorjones: I see much less than 1/3rd—about 1% is spent allocating XPathContexts, but closer to 20% registering namespaces (XPathContext#register_namespaces). This is probably because I'm working with OpenOffice documents, which have quite a number.

I'm using ruby-prof to profile. It's difficult to demonstrate (proprietary codebase, woo), but here's some stuff anyway:

10,095 calls to Nokogiri::XML::Node#xpath, consuming 36.49s (total), only 0.13s is in the #xpath body itself, excluding blocks. 28.98s of runtime in Array#map from #xpath. Of that, only 0.16s is in Array#map itself—rest in thus in the block that #map calls.

Note that very little time is spent in XPathContext's new; the biggest saving from caching them comes from not repeating namespace registration.

I worked around this by only registering namespaces I need (i.e. by collecting the ones I want beforehand and passing them into every #xpath invocation).

$ nokogiri -v
# Nokogiri (1.5.5)
    ---
    warnings: []
    nokogiri: 1.5.5
    ruby:
      version: 1.9.3
      platform: x86_64-darwin11.3.0
      description: ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-darwin11.3.0]
      engine: ruby
    libxml:
      binding: extension      compiled: 2.7.3      loaded: 2.7.3
etm commented 12 years ago

@flavorjones, @unnali: yes register_namespaces is also slow, but something else is going on - please try the following example yourself:

    require 'rubygems' 
    require 'nokogiri'

    module Nokogiri
      module XML
          class Node

            def xpath_fast1(path,prefixes=[])
              return NodeSet.new(document) unless document
              ctx = XPathContext.new(self)
              ctx.register_namespaces(prefixes) unless prefixes.empty?
              path = path.gsub(/xmlns:/, ' :') unless Nokogiri.uses_libxml?
              ctx.evaluate(path)
            end

            def xpath_fast2(path,prefixes=[])
              return NodeSet.new(document) unless document
              if @custom_prefixes != prefixes
                @ctx = XPathContext.new(self)
                @ctx.register_namespaces(prefixes) unless prefixes.empty?
                @custom_prefixes = prefixes
              end
              path = path.gsub(/xmlns:/, ' :') unless Nokogiri.uses_libxml?
              @ctx.evaluate(path)
            end

          end
      end
    end

    doc = Nokogiri.XML("<foo><baz/><baz><bar name='test'/></baz></foo>")

    s = Time.now.to_f
    1.upto 50000 do |i|
      doc.root.xpath_fast1('//baz')
    end
    puts Time.now.to_f - s

    s = Time.now.to_f
    1.upto 50000 do |i|
      doc.root.xpath_fast2('//baz')
    end
    puts Time.now.to_f - s

The results (obviously no difference between ruby 1.9.3 und ruby 1.8.7 - almost the same numbers as most of the stuff is done by nokogiri:

fast1: 2.12183022499084 fast2: 0.841907024383545

Thats a massive speedup. Also note that NO namespace prefixes are used here, with lots of namespace prefixes the results are even better.

Sorry for using old-fashioned monkey-patching and time measurement ;-)

p.s.:

# Nokogiri (1.5.5)
    --- 
    nokogiri: 1.5.5
    warnings: []

    libxml: 
      binding: extension
      compiled: 2.7.8
      loaded: 2.7.8
    ruby: 
      engine: mri
      platform: i686-linux
      version: 1.8.7
      description: ruby 1.8.7 (2011-06-30 patchlevel 352) [i686-linux]```

# Nokogiri (1.5.5)
    ---
    warnings: []
    nokogiri: 1.5.5
    ruby:
      version: 1.9.3
      platform: i686-linux
      description: ruby 1.9.3p0 (2011-10-30 revision 33570) [i686-linux]
      engine: ruby
    libxml:
      binding: extension
      compiled: 2.7.8
      loaded: 2.7.8```
etm commented 12 years ago

p.s.s. The problem that it is not reliable still remains (see comment 3 days ago). BUT, unless i'm doing something wrong, i think the numbers make this a worthwhile pursuit ;-). I'am looking into the reliability issue by adding #node for resetting in C - but i need some more time here.

kivikakk commented 12 years ago

Okay, so in your case the slowdown is because 50,000 new XPathContexts take half a second to allocate (xpath_fast1 only):

 %self     total     self     wait    child    calls   name
 23.53      0.83     0.58     0.00     0.25    50000   Nokogiri::XML::XPathContext#evaluate 
 20.23      0.49     0.49     0.00     0.00    50000   <Class::Nokogiri::XML::XPathContext>#new 
 18.92      2.21     0.46     0.00     1.75    50000   Nokogiri::XML::Node#xpath_fast1 
 10.23      0.25     0.25     0.00     0.00    50001   Nokogiri::XML::Document#decorate 
  5.57      0.32     0.14     0.00     0.18    50000   <Module::Nokogiri>#uses_libxml? 

Whether this is totally ridiculous or not, I can't really comment.

jvshahid commented 11 years ago

Duplicate of #741. This pr #1004 has a fix and will be merged into master soon.

Tristramg commented 9 years ago

@jvshahid correct me if I’m wrong, but #1004 only concerns JRuby, while @etm was talking about mri.

I observed also a speedup with the suggested monkey patching, nokogiri 1.6.6.2 and ruby mri 2.1.0

etm commented 9 years ago

Yes. I'm talking about MRI. And the (preceived) problem still persists. And its not primarily about speedup (which could be reaped after solving this). Its about the behaviour of the following code:

require 'rubygems'
require 'nokogiri'

doc = Nokogiri.XML("<foo><baz/><baz><bar name='test'/></baz></foo>")
xpa = Nokogiri::XML::XPathContext.new(doc.root)

p xpa.evaluate('//baz').length # => 2
p xpa.evaluate('baz').length # => 2
p xpa.evaluate('foo').length # => 0 
p xpa.evaluate('//@name').length # => 1
p xpa.evaluate('//baz').length # => 2
p xpa.evaluate('baz').length # => 0
p xpa.evaluate('foo').length # => 1  

This code should output 2201220, it does output 2201201. I still think this results point at a bug.

flavorjones commented 9 years ago

Urgh, OK, reopening.

@etm - I'm curious why no objections were raised when this ticket was closed. Honestly, I thought this had been solved, and apologize for the confusion.

etm commented 9 years ago

Sorry, entirely my fault, must have missed the mail notification that it was closed :-) It bugs me not that much that I check back regularely. After all, it only occurs in a code path, that is currently not used that way in nokogiri, so this is understandably not high priority.

flavorjones commented 9 years ago

@etm There is definitely some weirdness going on here. Part of the issue is that libxml2 stores state in XPathContext, meaning that you shouldn't re-use it unless you know what you're doing. Calling Node#xpath will create a new context each time to avoid these stateful behaviors.

This is also probably the behavior that's causing slowness.

That aside, there's still something else going on in the JRuby port that I don't fully understand yet; I need to isolate the behavior with test cases. Wish I could be more specific at this time.

flavorjones commented 4 months ago

For whatever it's worth the test case provided above now works properly on modern libxml2/nokogiri:

#! /usr/bin/env ruby

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "nokogiri"
end

RUBY_VERSION # => "3.3.3"
Nokogiri::VERSION_INFO
# => {"warnings"=>[],
#     "nokogiri"=>
#      {"version"=>"1.16.6",
#       "cppflags"=>
#        ["-I/home/flavorjones/.rbenv/versions/3.3.3/lib/ruby/gems/3.3.0/gems/nokogiri-1.16.6-x86_64-linux/ext/nokogiri",
#         "-I/home/flavorjones/.rbenv/versions/3.3.3/lib/ruby/gems/3.3.0/gems/nokogiri-1.16.6-x86_64-linux/ext/nokogiri/include",
#         "-I/home/flavorjones/.rbenv/versions/3.3.3/lib/ruby/gems/3.3.0/gems/nokogiri-1.16.6-x86_64-linux/ext/nokogiri/include/libxml2"],
#       "ldflags"=>[]},
#     "ruby"=>
#      {"version"=>"3.3.3",
#       "platform"=>"x86_64-linux",
#       "gem_platform"=>"x86_64-linux",
#       "description"=>
#        "ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [x86_64-linux]",
#       "engine"=>"ruby"},
#     "libxml"=>
#      {"source"=>"packaged",
#       "precompiled"=>true,
#       "patches"=>
#        ["0001-Remove-script-macro-support.patch",
#         "0002-Update-entities-to-remove-handling-of-ssi.patch",
#         "0003-libxml2.la-is-in-top_builddir.patch",
#         "0009-allow-wildcard-namespaces.patch",
#         "0010-update-config.guess-and-config.sub-for-libxml2.patch",
#         "0011-rip-out-libxml2-s-libc_single_threaded-support.patch"],
#       "memory_management"=>"ruby",
#       "iconv_enabled"=>true,
#       "compiled"=>"2.12.8",
#       "loaded"=>"2.12.8"},
#     "libxslt"=>
#      {"source"=>"packaged",
#       "precompiled"=>true,
#       "patches"=>
#        ["0001-update-config.guess-and-config.sub-for-libxslt.patch"],
#       "datetime_enabled"=>true,
#       "compiled"=>"1.1.39",
#       "loaded"=>"1.1.39"},
#     "other_libraries"=>{"zlib"=>"1.3.1", "libgumbo"=>"1.0.0-nokogiri"}}

doc = Nokogiri.XML("<foo><baz/><baz><bar name='test'/></baz></foo>")
xpa = Nokogiri::XML::XPathContext.new(doc.root)

xpa.evaluate('//baz').length # => 2
xpa.evaluate('baz').length # => 2
xpa.evaluate('//@name').length # => 1
xpa.evaluate('baz').length # => 2
xpa.evaluate('//baz').length # => 2
xpa.evaluate('foo').length # => 0
flavorjones commented 4 months ago

As for improving the performance of XPath queries, yes, we could re-use XPathContext objects but we do need to be careful about thread safety.

If someone wants to take a stab at writing something like an xpath-context-pool I'd happily review a PR.

flavorjones commented 4 months ago

I've opened a new issue to drive exploring XPath performance with respect to re-using context objects: https://github.com/sparklemotion/nokogiri/issues/3266

Closing this one in preference to that one.

kivikakk commented 4 months ago

What a blast from the past!