Performance regression

[jacobat]

Parsing https://www.e-conomic.com/secure/api1/EconomicWebservice.asmx?WSDL with Wasabi 1.0 takes ~0.3 sec:

require 'rubygems'
gem 'wasabi', '1.0.0'
require 'wasabi'
require 'benchmark'

wsdl = Wasabi.document File.read('economic.wsdl'); nil
Benchmark.measure do wsdl.endpoint end
 => #<Benchmark::Tms:0x10123ef90 @total=0.33, @cutime=0.0, @label="", @stime=0.01, @real=0.327524900436401, @utime=0.32, @cstime=0.0>

The same with 2.1.0 takes ~7sec:

require 'rubygems'
gem 'wasabi', '2.1.0'
require 'wasabi'
require 'benchmark'

wsdl = Wasabi.document File.read('economic.wsdl'); nil
Benchmark.measure do wsdl.endpoint end
 => #<Benchmark::Tms:0x1046a2a98 @label="", @stime=0.03, @real=6.99103879928589, @utime=6.97, @cstime=0.0, @total=7.0, @cutime=0.0>

[jacobat]

The regression was introduced with b1e76915fe73aae5861d1cd1d42ee200dc152e43

[jacobat]

It seems the issue is with the issue of Nokogiris collect_namespaces method. Apparently using this method is a bad idea, from the nokogiri documentation at http://nokogiri.org/Nokogiri/XML/Document.html#method-i-collect_namespaces :

Note this is a very expensive operation in current implementation, as it traverses the entire graph, and also has to bring each node accross the libxml bridge into a ruby object.

Any ideas how to improve the situation?

/cc @jkingdon

[jacobat]

A simple patch is https://github.com/jacobat/wasabi/commit/90c0339124dfb4a1759294fa748986688f4fca96 - not sure if it is acceptable though.

[jkingdon]

I don't have any particular ability to answer for all WSDL files out there whether the namespaces must be on the root node, nor did I notice such a restriction in a quick glance at http://www.w3.org/TR/wsdl but I would think this patch is worth a try. My guess would be that namespaces on the root node would be the most common practice. But perhaps someone knows better than I on such matters.

[jacobat]

I'm not sure what the best approach is. Perhaps it would be better if Wasabi was somehow able to cache the results of it's parsing. I have no idea how much data is actually extracted from the WSDL by Wasabi, but it seems wasteful to parse the whole WSDL like Savon does everytime a new client instance is created.

[rubiii]

interesting find. thanks for reporting! wasabi should cache every expensive parse-operation, but collect_namespaces just doesn't seem to be the right solution here.

[rubiii]

published v2.1.1 to fix this problem.

[koppen]

This seems to be an issue still (actually, it seems to have gotten worse). Here are the numbers I get from running the above code in Ruby 1.8.7 on different versions of Wasabi:

1.0.0: #<Benchmark::Tms:0x103ae96e8 @real=0.494114875793457, @utime=0.46, @cstime=0.0, @total=0.49, @cutime=0.0, @label="", @stime=0.03>
2.1.0: #<Benchmark::Tms:0x104882110 @real=8.68305802345276, @cutime=0.0, @label="", @total=8.69, @stime=0.05, @utime=8.64, @cstime=0.0>
2.4.0: #<Benchmark::Tms:0x101872740 @real=33.4192819595337, @stime=0.07, @utime=33.35, @total=33.42, @cstime=0.0, @cutime=0.0, @label="">

Numbers from Ruby 1.9.3 are almost as bad:

1.0.0:  0.430000   0.030000   0.460000 (  0.462021)
2.1.0:  7.070000   0.090000   7.160000 (  7.166233)
2.4.0: 28.290000   0.140000  28.430000 ( 28.432920)

[koppen]

Looks like 583cf658f1953411a7a7a4c22923fa0a046c8d6d introduced the 30ish second parsetime, and doesn't use collect_namespaces, so this is a new performance regression, not the one originally fixed, it seems.

[rubiii]

@hoverlover would you mind looking into this?

[hoverlover]

Sure, I can take a look.

[trevorhiley]

Any update on this one?

[hoverlover]

Sorry, I haven't had a moment to dig into this yet. Will try to look at it this weekend, but I'm going on vacation next week so it might be a little while.

[rubiii]

i looked into it. it's damn slow, but it's doing actual work. so i'm not sure how to optimize it right now. but it's a pretty significant regression, so we need to do something about it.

[rubiii]

since this change parses almost the entire document, it might make sense to switch back to a sax parser.

[hoverlover]

I'm sure a SAX parser would help, since it doesn't hold the entire document in memory. Not sure how it would affect speed. It would be worth a shot to create a test branch.

[rubiii]

looking into it ...

[jkingdon]

Didn't do any benchmarking when I switched us from SAX to Nokogiri, but Nokogiri is pretty fast so my first instinct is that it is usually going to be our code which is the bottleneck rather than parsing.

If we do want SAX, I think we want something like saxophone ( http://jkingdon2000.blogspot.com/2008/05/parse-xml-with-saxophone.html ). Trying to hack up the previous SAX parser to know about namespaces (and various other features of the WSDL which were needed at that time) was seriously painful.

[rubiii]

@jkingdon i think you're right. i played with it for a couple hours and can see this getting messy real quick. i'll take another look at the code and try to find to find the exact problem.

[koppen]

I can't say I entirely understand what the change actually does, but as far as I can tell, I haven't had a need for it in my projects - thus I would be perfectly happy not having to incur the parse time penalty (admittedly, I only use Savon for one endpoint/WSDL).

If it turns out to be too cumbersome/impossible to optimize this, we could perhaps consider making it an option?

[rubiii]

not every change to the parser affects every wsdl. reverting this change doesn't seem to be an option.

i started working on a sax parser which is actually pretty readable: https://github.com/savonrb/wasabi/commit/fc27d4a58a3814b61de7a6c7838fd287f40969a1

keeping state is kind of ugly, but i'm sure this can be improved somehow. the impl. has only been tested against one wsdl and it currently ignores xs schema types.

the idea here is to parse the wsdl into a format that can be interpreted by some other class. the benefit of this approach, is that the parser stays as slim as possible and the output can be dumped to a yaml file, marshalled and stored in redis or what have you. so apart from only reading the wsdl once, the parsed data can actually be cached as well.

my next step would be to test against other wsdl files and then build something that interprets the data to make this pass the existing specs.

[rubiii]

one other feature i was thinking about to improve performance, is to somehow remove conditionals from the giant case statement when they're not needed anymore. i don't know if that would make a huge difference and maybe the implementation for this would be way too complicated (procs?), but it might be worth a try.

[rubiii]

fyi: parsing the original wsdl takes about 5.4 sec on my mba. i expect that number to double since the current impl. doesn't parse wsdl:message nodes and wsdl:types yet.

[jkingdon]

What you've built on SAX (the stack, Wasabi::Matcher, etc) strike me as the kind of thing that lets one use SAX without turning the code into spaghetti (not drastically different from what I did in saxophone, which I think I mentioned to you). It reads nicely.

As for how to make all that matching performant, the holy grail I suppose is perhaps compiling the patterns to some kind of state machine or the like. But perhaps something easier, like a fast-path expression which recognized many of the nodes which weren't going to match anything, would get much of the benefit for less work. Even just keeping the code as it is and reordering the match statements to put the ones which match most often first might make a significant difference.

On 07/19/2012 07:07 AM, Daniel Harrington wrote:

not every change to the parser affects every wsdl. reverting this change doesn't seem to be an option.

i started working on a sax parser which is actually pretty readable: https://github.com/rubiii/wasabi/commit/fc27d4a58a3814b61de7a6c7838fd287f40969a1

keeping state is kind of ugly, but i'm sure this can be improved somehow. the impl. has only been tested against one wsdl and it currently ignores xs schema types.

the idea here is to parse the wsdl into a format that can be interpreted by some other class. the benefit of this approach, is that the parser stays as slim as possible and the output can be dumped to a yaml file, marshalled and stored in redis or what have you. so apart from only reading the wsdl once, the parsed data can actually be cached as well.

my next step would be to test against other wsdl files and then build something that interprets the data to make this pass the existing specs.

---

Reply to this email directly or view it on GitHub: https://github.com/rubiii/wasabi/issues/7#issuecomment-7099049

[rubiii]

saxophone looks nice and simple, but i would like to avoid pulling in another dependency for something this easy. i simplified and memoized the matchers and got the time down from 5.4 sec to 1.3 sec.

[jacobat]

If you need another big wsdl to test against you can try ExactTargets at: https://webservice.exacttarget.com/etframework.wsdl

[rubiii]

thanks @jacobat. i'll definitely use it for testing. but in terms of file size, the wsdl is only 197K, where e-conomic is 2.8M. and i still haven't found a larger wsdl than the one provided by ebay, which is 4.6M. quite insane, but perfect for performance testing :)

[jkingdon]

Saxophone, at least so far, was more of a proof of concept than something polished. So your matcher looks to be a better (or at least as good) implementation of a similar concept. I could see extracting your matcher as a separate gem, perhaps, but there are some issues in terms of how general purpose it is (for example, requiring the stack depth of the thing being matched to equal the matcher's depth would appear to be an important performance optimization which works as long as the patterns can be written that way, but which might not work for some tasks).

Not sure I fully agree with "easy". Although it isn't a lot of code, we've both seen how much spaghetti can result from trying to do without something like the stack/matcher concept.

[rubiii]

"easy" might be the wrong word :) but it's not as bad as i imagined it to be.

checking the stack size is a huge performance factor and the current impl. is certainly tailored to a special use case. right now i just want to concentrate on the sax experiment. but i can definitely see the need for a library like this. so why don't you push saxophone to github and post about it on rubyflow to see if people would be using it?!

[hoverlover]

Hey @rubiii. An acquaintance of mine wrote a blog post a while back on a subject very similar to the problem we are trying to solve. It's been a while since I've read it, but I think it might be relevant, and thought you might be interested in reading it.

[jkingdon]

@hoverlover: thanks, sax-machine does indeed look promising at least for many cases. I've blogged about it at http://jkingdon2000.blogspot.com/2012/07/parsing-large-xml-files.html .

[rubiii]

thanks for that interesting article @hoverlover. as far as i understand this, fibers were added to sax-machine to "lazy" parse values from an xml file. so, if processing a node takes a long time, i can see the benefit of this. please correct me if i'm wrong here.

i don't really see a way to improve the wasabi parser via fibers or threads, because as soon as you're executing a soap request, you pretty much need everything from the wsdl. namespaces, endpoints, operations and types.

right now, savon retrieves and parses the wsdl as soon as you need any of those values. i could imagine adding support for triggering this process "in the background" somehow. so, if for example, you're using savon inside a rails app, you could trigger this in an initializer and have wasabi prepare everything while your app is booting.

please let me know if i "just don't get it" :)

[hoverlover]

You're probably totally right. I'll admit it had been over a year since I read the article, and I only skimmed it to refresh my memory. Sorry if it wasn't relavent :)

Hunted and pecked from my iPhone. Please excuse any typos.

On Jul 21, 2012, at 11:15 AM, Daniel Harrington reply@reply.github.com wrote:

thanks for that interesting article @hoverlover. as far as i understand this, fibers were added to sax-machine to "lazy" parse values from an xml file. please correct me if i'm wrong here.

i don't really see a way to improve the wasabi parser via fibers or threads, because as soon as you're executing a soap request, you pretty much need everything from the wsdl. namespaces, endpoints and types.

right now, savon retrieves and parses the wsdl as soon as you need any of those values. i could imagine to add support for triggering this process "in the background" somehow. so, if for example, you're using savon inside a rails app, you could trigger this in an initializer and have wasabi prepare everything while the app is booting.

please let me know if i "just don't get it" :)

---

Reply to this email directly or view it on GitHub: https://github.com/rubiii/wasabi/issues/7#issuecomment-7154173

[rubiii]

well, i don't know if i'm right :) that's why i asked for feedback. what i know is, that i tried to improve the speed of the new sax-parser and that i don't see any obvious improvements right now. in contrast to the current implementation, the sax parser stores information about the entire wsdl document and it's actually quite fast. but if anyone has an idea about how to improve it, please let me know.

[koppen]

@henrik did some experimenting with using a marshalled WSDL instead of parsing it at runtime, significantly lowering process time.

He's got a gist of it at https://gist.github.com/4649195, perhaps something like that would be a good path to pursue? While it might not fix the issue, it should make it possible to work around it.

[henrik]

I've considered requesting/contributing some optimization to Savon proper, but marshalling specifically is a tricky problem.

I think they're supposed to be portable between Ruby implementations (MRI and JRuby, say), but I'm not sure about Ruby 1.8 vs. 1.9.

And there's the concern of compatibility across different versions of Wasabi. If you marshall the entire object as-is, the marshalled file will likely be very fragile.

I suppose if you discard and write a new marshal dump whenever the old one can't be loaded, you could get around these issues but would have to pay the parsing cost sometimes if you upgrade things.

A minor thing Wasabi could do is to not keep @document around after parsing, since it keeps the parser from being marshalled. That's one less thing to monkeypatch. Also, I've had some awful, awful inspects around Savon code because a representation of the 3 MB WSDL was somewhere in there. This would make it slightly better, though I suppose next time I should contribute a patch that changes how the object in question inspects.

I like what was said above about dumping the parsed WSDL to a YML or similar, and being able to load from that. If it was dumped in a format that (unlike a regular marshal dump) is not exactly mapped to the instance state, and if this was built into Wasabi, it could actually be fairly reliable across versions. So that would be great, but I suppose if a new SAX parser would make things faster anyway, we could wait and see what the performance is like after that.

[rubiii]

the new sax parser creates a simple hash-representation of the wsdl which could easily be marshalled. i haven't added support for caching yet, because like @henrik said, cache invalidation might be hard and i haven't had the time to think about it.

[rubiii]

ok, so ... i have lots of things to tell you about this whole parser-thing, but right now, i can only give you a quick update. i changed the current parser to benefit from caching and replaced a lot of very expensive xpath searches with walking the dom tree. sounds simple and in fact it just took some benchmarking and finding out how to avoid the slow parts of nokogiri.

when i started this, parsing the economic wsdl took about 34 seconds on my machine. just ridiculous. after these changes, it's down to 0.78 seconds!

here's the output from method_profiler before and after the refactoring: https://gist.github.com/rubiii/5484236

i'm quite satisfied with this, but i'm sure it could be even better. sadly, the code just sucks!

[rubiii]

if you could maybe point your gemfile to github master or otherwise test these changes, i'd really appreciate it!

[henrik]

Ooh, this sounds awesome. Will give it a go tomorrow at work!

On Mon, Apr 29, 2013 at 10:09 PM, Daniel Harrington < notifications@github.com> wrote:

if you could maybe point your gemfile to github master or otherwise test these changes, i'd really appreciate it!

— Reply to this email directly or view it on GitHubhttps://github.com/savonrb/wasabi/issues/7#issuecomment-17190871 .

[henrik]

@rubiii We're currently tied to Savon 0.9.5 because of another library that depends on that.

Seems we can't update just Wasabi on its own, because Savon 0.9.5 wants httpi (~> 0.9) and latest Wasabi wants httpi (~> 2.0).

Do you know off the top of your head if httpi 0.9 to 2.0 has any major breaking changes?

[rubiii]

@henrik not quite sure if the new httpi works with the old savon. could be that it depends on anything that changed, but i would give it a try. anything blocking you from updating to savon 2.0?

[rubiii]

@henrik here's the changelog.

[henrik]

Thanks! Oh yeah, the cookie stuff.

We could and should upgrade, but I don't think it'd be fast or painless. I could try this out on another smaller project, though.

[koppen]

@rubiii this sounds awesome, will check it out as soon as possible. Hopefully this means rconomic can finally upgrade to a newer savonrb.

[rubiii]

@koppen i would hope so as well. please let me know if there's anything i can do to help.

[henrik]

Alright, tested in with my tiny economic_api script where I'm experimenting with a low-level API wrapper.

On my Air:

With latest Wasabi (3.1.0 / 9d190edbd936184924794140ae215e98200a1fd6) and Savon (2.2.0 / 6a82e9b988f962f68e22979afbf41c31455867c6) and a test script that logs in and runs two requests, the best time I get is around 7 seconds all in all.

With my marshalling patch (which is definitely not production ready), the best times I get are just over 2 seconds.

Without either fix, the total run time is almost 40 seconds.

So it seems to work fine with the requests I tried, and it's a huge speedup. Looking forward to getting to a point where we can use it.

[rubiii]

@henrik thanks cool. are you doing anything fancy in your test script? because for me the time for parser.operations is down to 0.7s (and counting).

[henrik]

Nothing very fancy: https://gist.github.com/henrik/5488207

Not runnable without an e-conomic account, of course.

[henrik]

@rubiii Oh yeah, the retrieving from URL bit might be it. The marshalled version will of course only do that once, whereas it will probably do it every time with your patch, but parse it fast after. Let me check that…