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

Consider switching from underscore to lodash #182

Closed
jeremy-dentel opened this issue Apr 1, 2013 · 17 comments
Closed

Consider switching from underscore to lodash #182

jeremy-dentel opened this issue Apr 1, 2013 · 17 comments

Comments

@jeremy-dentel
Copy link

I did some testing and only one test seemed to fail, I forget which one, but I could easily rerun it and find out (and fix it too).

Lodash is compatible with the underscore API, and is actively developed. Its also much faster at times. Did the bench mark test at lodash.com (it seemed to be relatively unbiased in its results) and pulled out the key benchmark tests:

  • _.each: lodash was 27% to 215% faster
  • _.size: underscore is 5% faster
  • _.extend: lodash is 59% faster
  • _.filter: lodash is 69% to 353% faster
  • _.reduce: lodash is 73% to 143% faster
  • _.isNumber / _.isFunction: lodash is137% faster
  • _.map: lodash is 50% to 279% faster
  • _.bind: lodash is 24% faster
  • _.contains: lodash is 211% faster when iterating on an object and underscore is 3% faster when iterating on an array
  • _.reject: lodash is 68% to 279% faster

The following functions weren't benchmarked in the test:
_.isString and _.any

Here is the actual benchmark test results I received:
https://gist.github.com/jeremy-dentel/5287165

@jeremy-dentel
Copy link
Author

Did the switch and tested again. It fails at this test:

    var $apple = _($elems).filter(function(elem) {
      return elem.attribs['class'] === 'apple';
    });

In ./test/cheerio.js line 124 - 130

So I'm assuming _($elems) is working differently in lodash versus underscore.

@davidchambers
Copy link
Contributor

I believe @matthewmueller's preference is to go in the other direction: to remove Underscore entirely. I don't have a strong leaning in any direction.

@matthewmueller
Copy link
Member

+1 for removing underscore entirely. all the browser stuff isn't necessary for this lib. But, I do think lodash would be better than underscore. If you're going to go through and swap things out though, might as well remove the dep :-D

jeremy-dentel pushed a commit to jeremy-dentel/cheerio that referenced this issue Apr 1, 2013
Implementation of Lo-Dash that Fixes cheeriojs#182 for speed increases in
Cheerio.
@jeremy-dentel
Copy link
Author

I have no real favor to either, except that lo-dash is faster, so if one was to be used, I'd lean towards lo-dash versus underscore.

I might spend some time trying to remove the dependency as a project to just learn and figure out node.js and cheerio with a deeper understanding. Other than that, I do have a lo-dash implementation that works since lo-dash ships with an underscore lib.

@jeremy-dentel
Copy link
Author

@matthewmueller @davidchambers : I have a decent implementation without underscore, with slight increases in speed that doesn't depend on underscore except for parsing & one test file (_() isn't implemented). Would love some comments / testing. I plan on seeing if I can optimize a little bit before I make a pull request (after I get those two things not dependent on underscore).

@davidchambers
Copy link
Contributor

I'll leave this one to Matt. Personally, I'd rather have a well-maintained dependency than an internal module which implements some (but not all) of Underscore's functions.

@matthewmueller
Copy link
Member

Yah, I'm with David. Basically, what I had in mind was for every _.each(obj, fn) replace with obj.forEach(fn), _.map(obj, fn) => obj.map(fn), etc.

Now, maybe the library is too dependent on underscore now to get away with that, but it probably would just take some reworking with functions. It's been about a year since I switched off of underscore and so far I haven't found myself missing it.

Matt

On Apr 2, 2013, at 10:33 PM, David Chambers notifications@github.com wrote:

I'll leave this one to Matt. Personally, I'd rather have a well-maintained dependency than an internal module which implements some (but not all) of Underscore's functions.


Reply to this email directly or view it on GitHub.

@jeremy-dentel
Copy link
Author

@matthewmueller assume there is going to be native methods (I am honestly like 90% sure what is available). From what I changed, it shouldn't be that difficult to actually move away.

If staying for a library, I still recommend lo dash for speed increases.

@matthewmueller
Copy link
Member

maybe moving to lodash is the best option right now. It does seem a bit silly that .forEach is not as fast as _.forEach in lodash though.

On Apr 2, 2013, at 10:45 PM, Jeremy Dentel notifications@github.com wrote:

@matthewmueller assume there is going to be native methods (I am honestly like 90% sure what is available). From what I changed, it shouldn't be that difficult to actually move away.

If staying for a library, I still recommend lo dash for speed increases.


Reply to this email directly or view it on GitHub.

@jeremy-dentel
Copy link
Author

Just ran the tests on all 3 implementations:

  • master: 371ms
  • lodash: 361ms
  • reimplementing underscore: 364ms

I'm sure these speed differences may be faster on larger more complex test data. I just don't have the time to implement larger tests at this moment.

@sindresorhus
Copy link
Contributor

Looks like Lodash is still much faster than native .forEach: http://jsperf.com/lo-dash-each-vs-native-foreach/6 (underscore is the slowest)

Lodash is moving towards modularization, which would help getting the size down a bit for the browserified bundle.

@jdalton any eta on when it will be published?

@jdalton
Copy link

jdalton commented Sep 11, 2013

@sindresorhus Down to final unit testing before the bump, expect the bump in a day or so.

@matthewmueller
Copy link
Member

I'd like to move away from the underscore dependency altogether in the next big release. For now I'm fine with swapping in lodash.

@fb55
Copy link
Member

fb55 commented Feb 28, 2014

Since cheerio seems to be stuck with underscore, I just benchmarked it and lodash. Here are the results (as a diff - the added lines are lodash, the removed ones underscore):

diff --git a/benchmark/bench1.txt b/benchmark/bench2.txt
index 9479e18..b678e3c 100644
--- a/benchmark/bench1.txt
+++ b/benchmark/bench2.txt
@@ -1,144 +1,144 @@
 Test: Select all (file: jquery.html)
-   cheerio x 2,674 ops/sec ±1.18% (96 runs sampled)
+   cheerio x 2,925 ops/sec ±0.56% (100 runs sampled)
    Fastest: cheerio

 Test: Select some (file: jquery.html)
-   cheerio x 1,817 ops/sec ±0.72% (99 runs sampled)
+   cheerio x 1,860 ops/sec ±0.97% (99 runs sampled)
    Fastest: cheerio

 Test: manipulation - append (file: jquery.html)
-   cheerio x 1,935 ops/sec ±22.29% (26 runs sampled)
+   cheerio x 2,114 ops/sec ±19.29% (23 runs sampled)
    Fastest: cheerio

 Test: manipulation - prepend (file: jquery.html)
-   cheerio x 1,890 ops/sec ±18.57% (34 runs sampled)
+   cheerio x 2,029 ops/sec ±20.16% (31 runs sampled)
    Fastest: cheerio

 Test: manipulation - after (file: jquery.html)
-   cheerio x 1,956 ops/sec ±18.05% (32 runs sampled)
+   cheerio x 2,236 ops/sec ±20.85% (27 runs sampled)
    Fastest: cheerio

 Test: manipulation - before (file: jquery.html)
-   cheerio x 1,928 ops/sec ±13.95% (49 runs sampled)
+   cheerio x 2,034 ops/sec ±17.40% (35 runs sampled)
    Fastest: cheerio

 Test: manipulation - remove (file: jquery.html)
-   cheerio x 86,724 ops/sec ±2.23% (91 runs sampled)
+   cheerio x 87,687 ops/sec ±1.34% (95 runs sampled)
    Fastest: cheerio

 Test: manipulation - replaceWith (file: jquery.html)
-   cheerio x 1,285 ops/sec ±2.82% (95 runs sampled)
+   cheerio x 1,300 ops/sec ±1.84% (97 runs sampled)
    Fastest: cheerio

 Test: manipulation - empty (file: jquery.html)
-   cheerio x 416,084 ops/sec ±0.94% (94 runs sampled)
+   cheerio x 417,987 ops/sec ±0.31% (99 runs sampled)
    Fastest: cheerio

 Test: manipulation - html (file: jquery.html)
-   cheerio x 14,348 ops/sec ±0.39% (101 runs sampled)
+   cheerio x 13,887 ops/sec ±1.03% (99 runs sampled)
    Fastest: cheerio

 Test: manipulation - html render (file: jquery.html)
-   cheerio x 362 ops/sec ±0.86% (90 runs sampled)
+   cheerio x 345 ops/sec ±0.47% (86 runs sampled)
    Fastest: cheerio

 Test: manipulation - html independent (file: jquery.html)
-   cheerio x 15,249 ops/sec ±0.37% (99 runs sampled)
+   cheerio x 15,285 ops/sec ±0.79% (94 runs sampled)
    Fastest: cheerio

 Test: manipulation - text (file: jquery.html)
-   cheerio x 9,876 ops/sec ±0.80% (102 runs sampled)
+   cheerio x 10,023 ops/sec ±0.31% (101 runs sampled)
    Fastest: cheerio

 Test: traversing - Find (file: jquery.html)
-   cheerio x 644 ops/sec ±2.15% (92 runs sampled)
+   cheerio x 753 ops/sec ±0.99% (98 runs sampled)
    Fastest: cheerio

 Test: traversing - Parent (file: jquery.html)
-   cheerio x 18,003 ops/sec ±3.62% (93 runs sampled)
+   cheerio x 19,610 ops/sec ±1.78% (90 runs sampled)
    Fastest: cheerio

 Test: traversing - Parents (file: jquery.html)
-   cheerio x 61.31 ops/sec ±3.47% (66 runs sampled)
+   cheerio x 59.53 ops/sec ±3.62% (64 runs sampled)
    Fastest: cheerio

 Test: traversing - Closest (file: jquery.html)
-   cheerio x 126 ops/sec ±3.79% (74 runs sampled)
+   cheerio x 128 ops/sec ±3.67% (68 runs sampled)
    Fastest: cheerio

 Test: traversing - next (file: jquery.html)
-   cheerio x 28,608 ops/sec ±0.30% (100 runs sampled)
+   cheerio x 32,320 ops/sec ±0.44% (97 runs sampled)
    Fastest: cheerio

 Test: traversing - nextAll (file: jquery.html)
-   cheerio x 1,750 ops/sec ±1.00% (99 runs sampled)
+   cheerio x 1,849 ops/sec ±0.74% (100 runs sampled)
    Fastest: cheerio

 Test: traversing - nextUntil (file: jquery.html)
-   cheerio x 631 ops/sec ±0.61% (97 runs sampled)
+   cheerio x 652 ops/sec ±0.68% (98 runs sampled)
    Fastest: cheerio

 Test: traversing - prev (file: jquery.html)
-   cheerio x 28,166 ops/sec ±0.35% (99 runs sampled)
+   cheerio x 31,937 ops/sec ±0.32% (101 runs sampled)
    Fastest: cheerio

 Test: traversing - prevAll (file: jquery.html)
-   cheerio x 1,795 ops/sec ±0.55% (100 runs sampled)
+   cheerio x 1,895 ops/sec ±0.52% (101 runs sampled)
    Fastest: cheerio

 Test: traversing - prevUntil (file: jquery.html)
-   cheerio x 577 ops/sec ±0.36% (97 runs sampled)
+   cheerio x 616 ops/sec ±0.90% (97 runs sampled)
    Fastest: cheerio

 Test: traversing - siblings (file: jquery.html)
-   cheerio x 507 ops/sec ±2.52% (87 runs sampled)
+   cheerio x 684 ops/sec ±3.61% (78 runs sampled)
    Fastest: cheerio

 Test: traversing - Children (file: jquery.html)
-   cheerio x 2,975 ops/sec ±4.09% (83 runs sampled)
+   cheerio x 5,166 ops/sec ±1.69% (99 runs sampled)
    Fastest: cheerio

 Test: traversing - Filter (file: jquery.html)
-   cheerio x 10,570 ops/sec ±1.61% (96 runs sampled)
+   cheerio x 11,123 ops/sec ±1.83% (98 runs sampled)
    Fastest: cheerio

 Test: traversing - First (file: jquery.html)
-   cheerio x 2,143,356 ops/sec ±1.05% (98 runs sampled)
+   cheerio x 1,630,634 ops/sec ±0.69% (99 runs sampled)
    Fastest: cheerio

 Test: traversing - Last (file: jquery.html)
-   cheerio x 2,071,745 ops/sec ±0.50% (98 runs sampled)
+   cheerio x 1,616,869 ops/sec ±2.14% (94 runs sampled)
    Fastest: cheerio

 Test: traversing - Eq (file: jquery.html)
-   cheerio x 2,090,277 ops/sec ±0.85% (91 runs sampled)
+   cheerio x 1,619,388 ops/sec ±0.64% (100 runs sampled)
    Fastest: cheerio

 Test: attributes - Attributes (file: jquery.html)
-   cheerio x 11,017 ops/sec ±0.91% (96 runs sampled)
+   cheerio x 10,585 ops/sec ±3.12% (93 runs sampled)
    Fastest: cheerio

 Test: attributes - Single Attribute (file: jquery.html)
-   cheerio x 1,236,596 ops/sec ±0.51% (96 runs sampled)
+   cheerio x 1,201,997 ops/sec ±0.53% (96 runs sampled)
    Fastest: cheerio

 Test: attributes - Data (file: jquery.html)
-   cheerio x 269,154 ops/sec ±3.37% (88 runs sampled)
+   cheerio x 292,021 ops/sec ±1.16% (100 runs sampled)
    Fastest: cheerio

 Test: attributes - Val (file: jquery.html)
-   cheerio x 1,112 ops/sec ±3.11% (93 runs sampled)
+   cheerio x 1,142 ops/sec ±3.37% (93 runs sampled)
    Fastest: cheerio

 Test: attributes - Has class (file: jquery.html)
-   cheerio x 17,486 ops/sec ±1.23% (98 runs sampled)
+   cheerio x 19,887 ops/sec ±1.93% (101 runs sampled)
    Fastest: cheerio

 Test: attributes - Toggle class (file: jquery.html)
-   cheerio x 8,398 ops/sec ±0.30% (102 runs sampled)
+   cheerio x 8,451 ops/sec ±0.77% (101 runs sampled)
    Fastest: cheerio

 Test: attributes - Add Remove class (file: jquery.html)
-   cheerio x 1,314 ops/sec ±3.11% (89 runs sampled)
+   cheerio x 3,757 ops/sec ±0.35% (101 runs sampled)
    Fastest: cheerio

Originally, I wanted to open a pull request, but there's a failing test:

1) cheerio should be able to select multiple elements: $(".apple, #fruits"):
     Error: expected { __chain__: false,
  __wrapped__: 
   [ { type: 'tag',
       name: 'li',
       attribs: [Object],
       children: [Object],
       next: [Object],
       prev: null,
       parent: [Object],
       data: {} } ] } to have a property 'length'
      at Assertion.assert (cheerio/node_modules/expect.js/index.js:96:13)
      at Assertion.property (cheerio/node_modules/expect.js/index.js:381:12)
      at Assertion.length (cheerio/node_modules/expect.js/index.js:343:30)
      at testAppleSelect (cheerio/test/cheerio.js:59:28)
      at Context.<anonymous> (cheerio/test/cheerio.js:157:5)
      at Test.Runnable.run (cheerio/node_modules/mocha/lib/runnable.js:221:32)
      at Runner.runTest (cheerio/node_modules/mocha/lib/runner.js:374:10)
      at cheerio/node_modules/mocha/lib/runner.js:452:12
      at next (cheerio/node_modules/mocha/lib/runner.js:299:14)
      at cheerio/node_modules/mocha/lib/runner.js:309:7
      at next (cheerio/node_modules/mocha/lib/runner.js:247:23)
      at Object._onImmediate (cheerio/node_modules/mocha/lib/runner.js:276:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

In case someone wants to investigate this issue: Please check the lodash branch.

@jdalton
Copy link

jdalton commented Feb 28, 2014

In case someone wants to investigate this issue: Please check the lodash branch

Lo-Dash has a different style of chaining than Underscore. It chains by default when using _(..) where as Underscore requires _(..).chain(..). If you're looking for a better Underscore compat there is require('lodash/dist/lodash.underscore').

@fb55 fb55 mentioned this issue Mar 1, 2014
@fb55
Copy link
Member

fb55 commented Mar 1, 2014

@jdalton That solved it. Thanks!

@jugglinmike
Copy link
Member

Resolved via #404

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

No branches or pull requests

7 participants