Simplified _.max and _.min #1423

Merged
merged 2 commits into from Mar 3, 2014

Conversation

Projects
None yet
5 participants
@megawac
Collaborator

megawac commented Jan 27, 2014

Bringing this back up - the last time I saw this being discussed is #578.

Curious on thoughts as given a comparitor case is the more common use case from my experience with the functions.

Removing the native apply also normalizes the output for some oddities mentioned in #728 when given a positive number as the first argument and make the functions more consistent in general (for absurdly large collections)

E.g.

_.max([1, NaN]) //1 instead of NaN

Updated jsperf

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Jan 27, 2014

Owner

We want to be able to use the native implementation as much as possible — that's most of the point of the function existing in the first place.

Owner

jashkenas commented Jan 27, 2014

We want to be able to use the native implementation as much as possible — that's most of the point of the function existing in the first place.

@jashkenas jashkenas closed this Jan 27, 2014

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 19, 2014

Contributor

Ping @jashkenas, not sure if you saw my comment as it was in the OPs branch.

Contributor

jdalton commented Feb 19, 2014

Ping @jashkenas, not sure if you saw my comment as it was in the OPs branch.

@jashkenas jashkenas reopened this Feb 19, 2014

@davidchambers

View changes

underscore.js
each(obj, function(value, index, list) {
var computed = iterator ? iterator.call(context, value, index, list) : value;
- computed > result.computed && (result = {value : value, computed : computed});
+ if(computed > computedMax) {

This comment has been minimized.

Show comment Hide comment
@davidchambers

davidchambers Feb 19, 2014

Contributor

You could add a space after if.

@davidchambers

davidchambers Feb 19, 2014

Contributor

You could add a space after if.

@davidchambers

View changes

underscore.js
each(obj, function(value, index, list) {
var computed = iterator ? iterator.call(context, value, index, list) : value;
- computed < result.computed && (result = {value : value, computed : computed});
+ if(computed < computedMin) {

This comment has been minimized.

Show comment Hide comment
@davidchambers

davidchambers Feb 19, 2014

Contributor

Here, too.

@davidchambers

davidchambers Feb 19, 2014

Contributor

Here, too.

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 19, 2014

Contributor

@megawac Some of the changes here overlap those already done by #1448, might be worth rebasing them in.

Contributor

jdalton commented Feb 19, 2014

@megawac Some of the changes here overlap those already done by #1448, might be worth rebasing them in.

Simplify _.max and _.min
Remove the native apply case
@megawac

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Feb 20, 2014

Collaborator

Thanks for the ping - rebased to use the code that was changed in davidchambers' (or was it brandon's?) and mehdishojae's commits since the original PR. Thanks @jdalton

Collaborator

megawac commented Feb 20, 2014

Thanks for the ping - rebased to use the code that was changed in davidchambers' (or was it brandon's?) and mehdishojae's commits since the original PR. Thanks @jdalton

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Feb 20, 2014

Owner

@megawac — Would it be possible to see a quick jsperf for this patch vs. the current master?

Owner

jashkenas commented Feb 20, 2014

@megawac — Would it be possible to see a quick jsperf for this patch vs. the current master?

underscore.js
_.max = function(obj, iterator, context) {
- if (!iterator && _.isArray(obj) && obj[0] === +obj[0] && obj.length < 65535) {
- return Math.max.apply(Math, obj);
- }
var result = -Infinity, lastComputed = -Infinity;
each(obj, function(value, index, list) {

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 20, 2014

Contributor

I have a feeling that each use alone will not be enough to show a perf win. We can confirm with a jsPerf, but I think the Math.max and Math.min branch will need to be replaced with a simple for-loop branch.

@jdalton

jdalton Feb 20, 2014

Contributor

I have a feeling that each use alone will not be enough to show a perf win. We can confirm with a jsPerf, but I think the Math.max and Math.min branch will need to be replaced with a simple for-loop branch.

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Feb 20, 2014

Collaborator

I updated the jsperf test with your suggestion and it obviously breaks compat

_.max = function(obj, iterator, context) {
    var result = -Infinity,
      lastComputed = -Infinity;
  for(var index = 0, l = obj.length; index < l; index++) {
    value = obj[index];
    computed = iterator ? iterator.call(context, value, index, list) : value;
    if (computed > lastComputed) {
      result = value;
      lastComputed = computed;
    }
  }
  return result;
};
@megawac

megawac Feb 20, 2014

Collaborator

I updated the jsperf test with your suggestion and it obviously breaks compat

_.max = function(obj, iterator, context) {
    var result = -Infinity,
      lastComputed = -Infinity;
  for(var index = 0, l = obj.length; index < l; index++) {
    value = obj[index];
    computed = iterator ? iterator.call(context, value, index, list) : value;
    if (computed > lastComputed) {
      result = value;
      lastComputed = computed;
    }
  }
  return result;
};

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 20, 2014

Contributor

In my own implementation I keyed the fast path off of:
if (callback == null && isArray(collection)) {

else it hits the slow path, each.

@jdalton

jdalton Feb 20, 2014

Contributor

In my own implementation I keyed the fast path off of:
if (callback == null && isArray(collection)) {

else it hits the slow path, each.

@megawac

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Feb 20, 2014

Collaborator

http://jsperf.com/simplified-max-min/2

Slightly faster in chrome and slower in ff from my quick tests

Collaborator

megawac commented Feb 20, 2014

http://jsperf.com/simplified-max-min/2

Slightly faster in chrome and slower in ff from my quick tests

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 20, 2014

Contributor

RE the jsperf: Yay \o/, for-loops win!

Contributor

jdalton commented Feb 20, 2014

RE the jsperf: Yay \o/, for-loops win!

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Feb 20, 2014

Contributor

@megawac
Could you modify your PR to use the for-loop in place of the Math.m** use.
Something like:

  _.max = function(obj, iterator, context) {
    var result = -Infinity, lastComputed = -Infinity;
    if (!iterator && _.isArray(obj)) {
      for (var i = 0, length = obj.length; i < length; i++) {
        value = obj[i];
        if (value > result) {
          result = value;
        }
      }
    } else {
      each(obj, function(value, index, list) {
        var computed = iterator ? iterator.call(context, value, index, list) : value;
        if (computed > lastComputed) {
          result = value;
          lastComputed = computed;
        }
      });
    }
    return result;
  };
Contributor

jdalton commented Feb 20, 2014

@megawac
Could you modify your PR to use the for-loop in place of the Math.m** use.
Something like:

  _.max = function(obj, iterator, context) {
    var result = -Infinity, lastComputed = -Infinity;
    if (!iterator && _.isArray(obj)) {
      for (var i = 0, length = obj.length; i < length; i++) {
        value = obj[i];
        if (value > result) {
          result = value;
        }
      }
    } else {
      each(obj, function(value, index, list) {
        var computed = iterator ? iterator.call(context, value, index, list) : value;
        if (computed > lastComputed) {
          result = value;
          lastComputed = computed;
        }
      });
    }
    return result;
  };
@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Mar 3, 2014

Owner

Branching out for a for-loop is certainly a nice micro-optimization, but probably not something that's quite appropriate for core Underscore...

Owner

jashkenas commented Mar 3, 2014

Branching out for a for-loop is certainly a nice micro-optimization, but probably not something that's quite appropriate for core Underscore...

@jashkenas jashkenas closed this Mar 3, 2014

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Mar 3, 2014

Contributor

The point was it avoids the hacky arguments length guards that Underscore is using and wins perf to boot with little more than a for-loop.

Contributor

jdalton commented Mar 3, 2014

The point was it avoids the hacky arguments length guards that Underscore is using and wins perf to boot with little more than a for-loop.

@megawac

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Mar 3, 2014

Collaborator

@jashkenas I agree that @jdalton's implementation does not fit underscores style1, but I also disagree with the appropriateness of this Math.m**.apply hack that has compatibility issues.

_.max([1,2,3,"test"]) !== _.max(["test",1,2,3]) currently in underscore whereas it will always be true with the hack removed. I think this case alone is unjustifiable for a perf win for the absolutely simplest inputs.

1: Aside it may be useful to have an optimized internal forIn and forArray that don't consider context

Collaborator

megawac commented Mar 3, 2014

@jashkenas I agree that @jdalton's implementation does not fit underscores style1, but I also disagree with the appropriateness of this Math.m**.apply hack that has compatibility issues.

_.max([1,2,3,"test"]) !== _.max(["test",1,2,3]) currently in underscore whereas it will always be true with the hack removed. I think this case alone is unjustifiable for a perf win for the absolutely simplest inputs.

1: Aside it may be useful to have an optimized internal forIn and forArray that don't consider context

@jdalton

This comment has been minimized.

Show comment Hide comment
@jdalton

jdalton Mar 3, 2014

Contributor

@jashkenas I agree that @jdalton's implementation does not fit underscores style1, but I also disagree with the appropriateness of this Math.m**.apply hack that has compatibility issues.

Other places in Underscore use for-loops. The benefit is clear; it avoids the hacks Underscore currently has without a negative perf impact (the reason Underscore is using the hacks in the first place).

Contributor

jdalton commented Mar 3, 2014

@jashkenas I agree that @jdalton's implementation does not fit underscores style1, but I also disagree with the appropriateness of this Math.m**.apply hack that has compatibility issues.

Other places in Underscore use for-loops. The benefit is clear; it avoids the hacks Underscore currently has without a negative perf impact (the reason Underscore is using the hacks in the first place).

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Mar 3, 2014

Owner

Alright then. @megawac Want to amend this PR?

Owner

jashkenas commented Mar 3, 2014

Alright then. @megawac Want to amend this PR?

@jashkenas jashkenas reopened this Mar 3, 2014

@megawac

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Mar 3, 2014

Collaborator

Sure I'll rebase did you want me to include @jdalton's suggestions (sorry about the last commit I'm a moron I'll cherry pick it out)

Collaborator

megawac commented Mar 3, 2014

Sure I'll rebase did you want me to include @jdalton's suggestions (sorry about the last commit I'm a moron I'll cherry pick it out)

@jashkenas

This comment has been minimized.

Show comment Hide comment
@jashkenas

jashkenas Mar 3, 2014

Owner

Yes. For simple arrays, use the for loop, otherwise, fall back to the each.

Owner

jashkenas commented Mar 3, 2014

Yes. For simple arrays, use the for loop, otherwise, fall back to the each.

}
- });
+ } else {
+ each(obj, function(value, index, list) {

This comment has been minimized.

Show comment Hide comment
@michaelficarra

michaelficarra Mar 3, 2014

Collaborator

Is there a generic fold we can use instead? I'm okay with implementing map using fold, but the other way around is just ... weird, since fold is a more basic abstraction.

@michaelficarra

michaelficarra Mar 3, 2014

Collaborator

Is there a generic fold we can use instead? I'm okay with implementing map using fold, but the other way around is just ... weird, since fold is a more basic abstraction.

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Mar 3, 2014

Collaborator

Something like this @michaelficarra (goes against #1448)?

_.min = function(obj, iterator, context) {
  return _.reduce(obj, function(last, value, index, list) {
    var computed = iterator ? iterator.call(context, value, index, list) : value;
    return computed < last.computed ? {value: value, computed: computed} : last;
  }, {result: Infinity, computed: Infinity}).value;
};
@megawac

megawac Mar 3, 2014

Collaborator

Something like this @michaelficarra (goes against #1448)?

_.min = function(obj, iterator, context) {
  return _.reduce(obj, function(last, value, index, list) {
    var computed = iterator ? iterator.call(context, value, index, list) : value;
    return computed < last.computed ? {value: value, computed: computed} : last;
  }, {result: Infinity, computed: Infinity}).value;
};

This comment has been minimized.

Show comment Hide comment
@michaelficarra

michaelficarra Mar 3, 2014

Collaborator

Yeah, though it's too bad it kills performance. Let's stick with what you have.

@michaelficarra

michaelficarra Mar 3, 2014

Collaborator

Yeah, though it's too bad it kills performance. Let's stick with what you have.

@megawac

This comment has been minimized.

Show comment Hide comment
@megawac

megawac Mar 3, 2014

Collaborator

@jashkenas I've updated the PR with jdalton's suggested implementation

Collaborator

megawac commented Mar 3, 2014

@jashkenas I've updated the PR with jdalton's suggested implementation

jashkenas added a commit that referenced this pull request Mar 3, 2014

Merge pull request #1423 from megawac/_.max
Simplified _.max and _.min

@jashkenas jashkenas merged commit 512854c into jashkenas:master Mar 3, 2014

1 check passed

default The Travis CI build passed
Details

@megawac megawac deleted the megawac:_.max branch Jun 11, 2014

@megawac megawac referenced this pull request Aug 27, 2014

Merged

Release Underscore 1.7.0 #1799

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