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

Performance #54

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

Performance #54

mhemesath opened this issue Jul 13, 2011 · 17 comments

Comments

@mhemesath
Copy link
Contributor

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

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

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

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

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

tj commented Jul 19, 2011

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

@mhemesath
Copy link
Contributor Author

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

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

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

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

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

tj commented Jul 20, 2011

no clue what that is

@mhemesath
Copy link
Contributor Author

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

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

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

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

masylum commented Aug 5, 2011

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

@mhemesath
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants