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

Adds ability to do Hoek.reach with negative index #115

Merged

Conversation

@danielb2
Copy link
Contributor

danielb2 commented Dec 5, 2014

Fixes #114

So I did benchmark this with three different versions.

Version 1 was plain without negative index.
Version 2 used an initial check to see if there's any negative in the string, plus a negative for each part.
Version 3 just does a check for a negative for each part of the path (v2 in test)

The result was this:

get with index x 3,005,799 ops/sec ±0.24% (1094 runs sampled)
get with negative index x 2,180,143 ops/sec ±0.21% (1091 runs sampled)
get with negative index v2 x 2,427,552 ops/sec ±0.25% (1094 runs sampled)
Fastest is get with index
      386.24 real       180.59 user         1.34 sys

See below for the code for the test. let me know if you'd like me to test it some other way, but I thought with 1000 samples something would have come up if it was significantly slower.

var Hoek = require('hoek');
var HoekNeg = require('hoek-neg');
var HoekNeg2 = require('hoek-neg2');
var Benchmark = require('benchmark');
var suite = new Benchmark.Suite;

var a = [1, 2, 3, 4, 5, 6, 7, 8];
suite.add('get with index', function () {


    var b = Hoek.reach(a, '0');
})

suite.add('get with negative index', function () {


    var b = HoekNeg.reach(a, '0');
})

suite.add('get with negative index v2', function () {


    var b = HoekNeg2.reach(a, '0');
})


suite.on('complete', function () {
    console.log('Fastest is ' + this.filter('fastest').pluck('name'));
});

suite.on('cycle', function (event) {
    console.log(String(event.target));
})

suite.run({ async: true, minSamples: 150, name: 'whatever'  });
@danielb2 danielb2 closed this Dec 5, 2014
@danielb2

This comment has been minimized.

Copy link
Contributor Author

danielb2 commented Dec 5, 2014

Anyway, I suspect it's not possible to do without performance impact, but this is what I tried.

@@ -549,17 +549,23 @@ exports.reach = function (obj, chain, options) {
var path = chain.split(options.separator || '.');
var ref = obj;
for (var i = 0, il = path.length; i < il; ++i) {
var key = path[i];
if (key.indexOf('-') === 0 && Array.isArray(ref)) {

This comment has been minimized.

Copy link
@Marsup

Marsup Dec 5, 2014

Member

I think key[0] === '-' would be faster, no need to go through the whole string if you only want the 1st letter.

This comment has been minimized.

Copy link
@nlf

nlf Dec 5, 2014

Member

Yup, that would likely speed it up a fair amount.

This comment has been minimized.

Copy link
@danielb2

danielb2 Dec 5, 2014

Author Contributor

gah, of course. thanks

@danielb2

This comment has been minimized.

Copy link
Contributor Author

danielb2 commented Dec 5, 2014

Changing to key[0] === '-' did improve things, but I don't know if it's acceptable. See below:

get with index x 3,129,504 ops/sec ±0.47% (1091 runs sampled)
get with negative index x 2,641,638 ops/sec ±0.30% (1091 runs sampled)
get with negative index v2 x 2,430,195 ops/sec ±0.24% (1097 runs sampled)
get with negative index (no index of) v3 x 2,694,016 ops/sec ±0.45% (1097 runs sampled)
Fastest is get with index

Going to try one more time. I think I got the wrong package.

@danielb2

This comment has been minimized.

Copy link
Contributor Author

danielb2 commented Dec 5, 2014

OK, here are the numbers:

get with index x 3,404,628 ops/sec ±0.33% (1094 runs sampled)
get with negative index x 2,833,493 ops/sec ±0.17% (1097 runs sampled)
get with negative index v2 x 2,753,768 ops/sec ±0.21% (1095 runs sampled)
get with negative index v3 x 3,458,322 ops/sec ±0.27% (1094 runs sampled)
Fastest is get with negative index v3

Looks like it would be a negligible performance hit now

@danielb2 danielb2 reopened this Dec 5, 2014
@nlf nlf added the feature label Dec 22, 2014
@nlf nlf self-assigned this Dec 22, 2014
@nlf nlf added this to the 2.11.0 milestone Dec 22, 2014
@nlf

This comment has been minimized.

Copy link
Member

nlf commented Dec 22, 2014

Can you update the docs for this also? @danielb2

@danielb2 danielb2 force-pushed the danielb2:issue.114-Hoek.reach.with.negative.index branch from a3513aa to 297d049 Dec 31, 2014
@danielb2

This comment has been minimized.

Copy link
Contributor Author

danielb2 commented Dec 31, 2014

done. sorry for taking so long, this email got buried

nlf added a commit that referenced this pull request Jan 5, 2015
…ive.index

Adds ability to do Hoek.reach with negative index
@nlf nlf merged commit d2b0f53 into hapijs:master Jan 5, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.