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

_.bind nontrivial constructor fix (?) #2845

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jgonggrijp
Copy link
Collaborator

A quick summary of what led up to this diff:

  • Slicing arguments causes huge performance drop in _.bind() #2197 was a complaint about _.bind performing worse than native Function.prototype.bind, due to it basically being slice.call(arguments) + native bind (at least at the time).
  • Remove nativeBind usage #2199 "fixed" this by completely removing native bind out of the equation (but didn't update the comment, which has continued to claim a fallback to native bind until this very day).
  • executeBound on var arg constructors #2214 reported that this was a breaking change: _.bind previously worked with nontrivial constructors such as Date in environments that supported native bind, but not anymore. This has been the status quo since June 2015.
  • In the discussion that followed, this was considered the most favorable solution: check for availability of native bind only at the time of definition of _.bind.
  • I took the above solution, updated it to the current situation and added a regression test.

I'm not actually sure this should be merged. Is using a bound function as a constructor important to support? On the other hand, #2199 was probably the wrong solution to the wrong problem in the first place, so maybe this detour should never have been made.

@jashkenas
Copy link
Owner

I'm not actually sure this should be merged. Is using a bound function as a constructor important to support?

It is not important to support. Within a constructor function, this should be the instance you’re constructing.

I feel like we should rely on native bind here if there are performance improvements it gives us, and not worry about it otherwise.

@jgonggrijp
Copy link
Collaborator Author

Here's the thing though. executeBound already has a fallthrough case for being called as a constructor (i.e. with new), in which case it doesn't keep the this binding but does keep the argument bindings:

underscore/modules/index.js

Lines 770 to 778 in 4334c12

// Determines whether to execute a function as a constructor
// or a normal function with the provided arguments.
function executeBound(sourceFunc, boundFunc, context, callingContext, args) {
if (!(callingContext instanceof boundFunc)) return sourceFunc.apply(context, args);
var self = baseCreate(sourceFunc.prototype);
var result = sourceFunc.apply(self, args);
if (isObject(result)) return result;
return self;
}

And it works for simple constructors, but not for fancy stuff like Date. Native bind has this sophistication too, but it does work for fancy constructors as well. So where to take this:

  • Support some constructors but not others (current situation)?
  • Support all constructors (this PR)?
  • Support no constructors at all (another breaking change)?

@jashkenas
Copy link
Owner

So where to take this:

  • Support some constructors but not others (current situation)?
  • Support all constructors (this PR)?
  • Support no constructors at all (another breaking change)?

I don’t believe that it’s terribly important — these sorts of tickets are originally driven by test-case, nit-picked development, not real-world use cases ... But, if it doesn’t cost us much (in bound function call speed, and in file size) to support all constructors with this PR, I think it’s fine and good to merge it!

@jgonggrijp
Copy link
Collaborator Author

Time for another benchmark, then.

@jgonggrijp jgonggrijp removed the request for review from jashkenas May 6, 2020 22:53
@jgonggrijp jgonggrijp marked this pull request as draft May 6, 2020 22:53
@jgonggrijp jgonggrijp self-assigned this Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants