Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for issue 578 (stack overflow in min and max) #591

Closed
wants to merge 2 commits into from

4 participants

@cederberg

Small patch to fix issue #578. Added try-catch to handle stack overflow when too many values are passed to min() or max() functions without an iterator.

@cederberg

Sorry about the mixup of by two patch branches. If you accept the doc-clarifications, then that should go first. Otherwise I'll have to rebase this one... :-(

@kitcambridge

Instead of adding a (rather costly) try...catch, might I suggest checking obj.length instead? 512 items should be a safe limit—IIRC, this is the maximum permissible length in Opera 9.x:

if (!iterator && _.isArray(obj) && obj[0] === +obj[0] && obj.length < 512) return Math.max.apply(Math, obj);
// ...
@cederberg

Problem with checking obj.length is that the stack sizes in any old browser are completely arbitrary. And it becomes an optimization for the lowest common denominator among browser stack sizes.

The try-catch solution has negligible (if any) cost when the limit is not exceeded. And when the stack size limit is hit (and an exception is thrown), the performance penalty is small compared to the cost of the iteration itself.

If you don't believe me regarding try-catch, try playing around with the following code:

var time = Date.now();
var arr = [];
for (var i = 0; i < 1000000; i++) {
//    try {
        arr.push(i);
//    } catch (e) {}
}
Date.now() - time
@jdalton

@cederberg This problem seems really really edge to me.
That said, I dig testing it :D

var array = [],
    limit = 0,
    noop = function() {};
try {
  while (++limit <= 1e3) {
    noop.apply(undefined, array);
    array.length = limit;
  }
} catch(e) {}

// later
if (!iterator && _.isArray(obj) && obj[0] === +obj[0] && obj.length < limit)

Needs a JSPerf, esp on mobile.

@cederberg

Well...

It's definitely interesting to optimize micro benchmark results. And there are clearly issues to be found with try-catch. But in this case, any such issues are still totally dwarfed by the cost of the actual operation to perform:

http://jsperf.com/try-block-performance

So I think correctness is the more important issue here. And optimizing this case just isn't worthwhile. The _.min() and _.max() functions are designed to take all values as input (looping internally). For tight loops, the built-in Math.min and Math.max are better matches anyway.

@cederberg

Whatever. Another fix is in the tree.

@cederberg cederberg closed this
@jashkenas
Owner

Fixed on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 3 deletions.
  1. +4 −1 index.html
  2. +14 −2 underscore.js
View
5 index.html
@@ -1148,13 +1148,16 @@ <h2 id="objects">Object Functions</h2>
<p id="isObject">
<b class="header">isObject</b><code>_.isObject(value)</code>
<br />
- Returns <i>true</i> if <b>value</b> is an Object.
+ Returns <i>true</i> if <b>value</b> is an Object. Returns
+ <i>false</i> for primitive types and <i>null</i>.
</p>
<pre>
_.isObject({});
=&gt; true
_.isObject(1);
=&gt; false
+_.map([null, 0, true, ""], _.isObject)
+=&gt; [false, false, false, false]
</pre>
<p id="isArguments">
View
16 underscore.js
@@ -225,7 +225,13 @@
// Return the maximum element or (element-based computation).
_.max = function(obj, iterator, context) {
- if (!iterator && _.isArray(obj) && obj[0] === +obj[0]) return Math.max.apply(Math, obj);
+ if (!iterator && _.isArray(obj) && obj[0] === +obj[0]) {
+ try {
+ return Math.max.apply(Math, obj);
+ } catch (e) {
+ // Too many values for apply, fall-trough to iteration
+ }
+ }
if (!iterator && _.isEmpty(obj)) return -Infinity;
var result = {computed : -Infinity};
each(obj, function(value, index, list) {
@@ -237,7 +243,13 @@
// Return the minimum element (or element-based computation).
_.min = function(obj, iterator, context) {
- if (!iterator && _.isArray(obj) && obj[0] === +obj[0]) return Math.min.apply(Math, obj);
+ if (!iterator && _.isArray(obj) && obj[0] === +obj[0]) {
+ try {
+ return Math.min.apply(Math, obj);
+ } catch (e) {
+ // Too many values for apply, fall-trough to iteration
+ }
+ }
if (!iterator && _.isEmpty(obj)) return Infinity;
var result = {computed : Infinity};
each(obj, function(value, index, list) {
Something went wrong with that request. Please try again.