Performance #54

Open
mhemesath opened this Issue Jul 13, 2011 · 17 comments

Projects

None yet

3 participants

@mhemesath

Tobi's performance really goes down as the page size increases. Some of my tests run on very large forms, and 30+ seconds can pass between requests triggered by Tobi.

The issue probably lies somewhere in JSDOM, or JQuery in tandem with JSDOM. As I get time I'll try to find out the exact bottleneck and ways to reduce it.

@tj
Member
tj commented Jul 13, 2011

yeah i haven't profiled myself, but I've heard bad things about JSDOM's perf, not sure though

@mhemesath

My tests seem to stress the most on Browser.fill. I'm wondering if we should consider taking Browser.locate and optimize it a bit. We can do that by attempting to retrieve the elements using the faster locators first, then exiting with the elements if result comes back.

Looking at the code below.. rather than running filter, maybe do each of the queries below until we detect elements. I don't think the assumption is too far off that people won't be using locators with the intent of them spanning multiple attributes?

Browser.prototype.locate = function(selector, locator){
  var self = this
    , $ = this.jQuery;

  // pseudo code. Try fastest searches first until something is found
  var elems = $(#locator)
  elems = elems.length < 1 ?  $("*[value=locator]") : elems;
  elems = elems.length < 1 ?  $("*[name=locator]") : elems;
  elems = elems.length < 1 ?  $("*").contains(locator) : elems;

  if (elems && !elems.length) throw new Error('failed to locate "' + locator + '" in context of selector "' + selector + '"');
  return elems;
};
@tj
Member
tj commented Jul 13, 2011

we should definitely profile first before considering anything, but yeah if there are obvious tweaks then I'm down, I dont even remember how I have things implemented

@mhemesath

Alright.. I have GOT to resolve this as this is driving me nutz! I'm feeling ambitious.. gonna try swapping JSDOM for PhantomJS. If this fails, I'm gonna profile the $hit out of this project :p

@tj
Member
tj commented Jul 19, 2011

definitely worth profiling first to see if it's the html parser, jsdom, etc

@mhemesath

Yeah, didn't get too far on swapping out the DOM impl. It will be tougher than I thought. Any ideas on a good profiling solution?

@tj
Member
tj commented Jul 19, 2011

v8 has a built in profiler, though I've never personally been able to get overly useful output from it, might have been doing something wrong, I think node-inspector (or something by the same guy) has profiling which I assume uses the same output

@mhemesath

So I did a bit of profiling around a fill method which was taking about 5 seconds to complete. It appears that

jquery.Fn.extend.find

which is called from Browser.locate is slow. This is from the call to

Sizzle.uniqueSort

which triggers the jsDOM method

compareDocumentPosition

in jsdom/level3/core.js on line 81 which takes 2700ms to execute.

I know this is a crappy profile report but it just spit out a giant JSON object. I'm going to create a viewer of some sort to help make sense of this all, so we can make a better assessment. For now it appears that the problem lies in JSDOM.

@tj
Member
tj commented Jul 20, 2011

oh wow, interesting, definitely a hint in the right direction. I dont really see how a js implementation should be (much) slower than a browser, that seems pretty brutal considering a browser needs to do similar many times a second

@mhemesath

Tobi doesn't override JSDOM's { QuerySelector : false } default value. I wonder what effect, if any, this would have. This may entirely change Sizzle's performance on JSDOM. Is there a reason QuerySelector was never enabled?

@tj
Member
tj commented Jul 20, 2011

no clue what that is

@mhemesath

Me neither, all it says is:

QuerySelector default : false allowed : true

This feature is backed by sizzle but currently causes problems with some libraries. Enable this if you want document.querySelector and friends, but be aware that many libraries feature detect for this, and it may cause you a bit of trouble.

@mhemesath

Drrr... stupid post. That would make JSDOM support document.querySelector. So basically, jQuery would be using sizzle packaged with JSDOM rather than sizzle packaged in itself.

@tj
Member
tj commented Jul 20, 2011

hmm, and the jsdom one is tweaked then im assuming? still seems really bizarre to be so slow, almost like you would have to try and make it that slow

@mhemesath

I don't know if the jsdom sizzle is tweaked. It may make sense using that one anyway in case it is. I'm not too surprised by the performance. My forms are fairly large, and from experience, jQuery selector performance can degrade rather quickly if the DOM size grows large and the selector isn't optimized. I'll keep investigating.

@masylum
masylum commented Aug 5, 2011

I'm having performance issues with Browser.fill, I will try to optimize it a little bit.

@mhemesath

JSDOM's latest release says it patched a main memory leaks. This may have been the culprit. https://gist.github.com/1129679

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