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

#xpath performance #760

Closed
etm opened this issue Sep 12, 2012 · 18 comments
Closed

#xpath performance #760

etm opened this issue Sep 12, 2012 · 18 comments

Comments

@etm
Copy link

etm commented Sep 12, 2012

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
Copy link

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
Copy link
Author

etm commented Oct 19, 2012

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
Copy link

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
Copy link
Member

@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
Copy link

@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
Copy link
Author

etm commented Oct 22, 2012

@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
Copy link
Author

etm commented Oct 22, 2012

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
Copy link

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
Copy link
Member

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

@Tristramg
Copy link

@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
Copy link
Author

etm commented Jan 27, 2015

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
Copy link
Member

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.

@flavorjones flavorjones reopened this Jan 28, 2015
@etm
Copy link
Author

etm commented Jan 28, 2015

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

I've opened a new issue to drive exploring XPath performance with respect to re-using context objects: #3266

Closing this one in preference to that one.

@kivikakk
Copy link

kivikakk commented Jul 2, 2024

What a blast from the past!

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

No branches or pull requests

5 participants