Consider not testing IsCallable on capability.[[Resolve]] and [[Reject]] until later

[domenic]

@getify pointed out in https://gist.github.com/domenic/43d3228e141ea225ea6e#comment-1229287 that the up-front checking of whether [[Resolve]] and [[Reject]] are callable may be a performance hit. I would be curious to hear from more implementers on this (@rossberg-chromium? @bzbarsky? Others?), but it seems plausible.

It may be worth trying to remove these checks, i.e. failing when they are called, instead of when they are created. This would be a subtle change, and have very few visible consequences, and only impact subclasses or Promise.staticMethod.call(OtherPromiseLikeConstructor, ...) cases. But it would have to be done carefully since [[Resolve]] and [[Reject]] are called quite often.

I think this fits better with @allenwb's general advice that in JavaScript, up-front type checks are not good, and you should just try to use the object in question. On the other hand, Array.prototype.map et al. do such up-front type checks, e.g. [].map(nonFunction) throws, even though nonFunction is never called.

This was previously discussed in #50, at which time the error-on-call approach was favored. But in the big rewrite of 002ec7a25a97c70eb131096bd3f39da26c6d87c4, it seems we switched to an error-on-creation approach. I also have a recollection that adding these checks was important to fix some kind of spec hole, but I can't find that anymore.

[bzbarsky]

Checking whether something is callable is pretty cheap (in SpiderMonkey it's a 3-pointer chase; the biggest issue is if you get a cache miss, but if we're talking about always being in the case when we have a Function (which we normally are) and doing the check often enough that the performance matters chances are everything involved will be in cache.

Past that I have no opinions, since I don't know when these are used. I can try to figure that out if that would be useful.

[allenwb]

I wouldn't read too much into how [].map works as the spec. simply describes the lbehavior of the original "array extras" implementation and it isn't clear whether these sorts of considerations were considered in that original design.

One consideration is the fact that [].map and similar functions have a loop within which the callback function is repeatedly invoked. It makes sense to factor the IsCallable test outside of the loop. That could be done in a way that avoids the test for 0-iteration loops but I have a hard time seeing a real justification for that extra complexity.

In general, I standby the advice to postpone such checks as long as possible. But that's just a guideline, and not an absolute rule. In most cases I probably wouldn't want to add complexity just to enable delaying a check.

[getify]

In most cases I probably wouldn't want to add complexity just to enable delaying a check.

I think it's the opposite, at least in this case. We're taking out checks from places where they could be considered unnecessary. :)

[allenwb]

I think it's the opposite, at least in this case. We're taking out checks from places where they could be considered unnecessary. :)

...and that's great. In this specific case I'm having a hard time being sure about the impact as it seems that a non-function occurrence could only arise via s subclass constructor and I haven't yet totally grokked that path..

The other consideration that specifically applies here relates to the difficulty of isolating asynchronous errors. Will deferring the check make it harder to trace an actual error back to the originating source?

[getify]

Will deferring the check make it harder to trace an actual error back to the originating source?

IIUC, deferring the check for [[Resolve]] will explicitly change the "error reporting" from being an immediately thrown error at the call-site of the method (like at Promise.resolve.call( OtherNonPromiseThingy, 2 )) to being triggered as a rejection of the promise when you attempt to resolve it.

Of course, if the problem is that [[Reject]] is trying to be called for a legitimately rejected promise (or because [[Resolve]] just failed!), and we find out that [[Reject]] can't be called because it's not a function, then we by definition cannot actually reject this promise, so I think this error would be stuck. What could happen at that point? Could it be thrown as a global error?

[cscott]

That send like a reasonable reason to check and throw immediately, rather than wait and reject. --scott On May 16, 2014 7:41 PM, "Kyle Simpson" notifications@github.com wrote:

Will deferring the check make it harder to trace an actual error back to the originating source?

IIUC, deferring the check for [[Resolve]] will explicitly change the "error reporting" from being a thrown-error at the call-site of the method (like Promise.resolve.call( OtherPromiseThingy, 2 )) to being triggered as a rejection of the promise when you attempt to resolve it.

Of course, if the problem is that [[Reject]] is trying to be called for a legitimately rejected promise (or because [[Resolve]] just failed!), and we find out that [[Reject]] can't be called because it's not a function, then we by definition cannot actually reject this promise, so I think this error would be stuck. What could happen at that point? Could it be thrown as a global error?

— Reply to this email directly or view it on GitHubhttps://github.com/domenic/promises-unwrapping/issues/107#issuecomment-43395524 .

[anba]

What could happen at that point? Could it be thrown as a global error?

Depends on the context, if it happens as part of a PromiseReactionTask, the result is implementation defined. For example V8 silently catches the error and does no error reporting at all. In different contexts, the error could/should probably be emitted through the standard throw facility.

[getify]

That send like a reasonable reason to check and throw immediately

May seem strange to suggest, but does that mean we could check [[Reject]] immediately, and throw if it's bad, but we could lazily defer the check on [[Resolve]]? Or would that inconsistency just be worse than any potential "savings" in removing the static check?

[domenic]

The conclusion here seems to be that the gains are very minor, if they exist at all, and the complications myriad. If someone is interested in investigating all the work and exact implications, perhaps we could consider this, but given the implementer comments, I can't see how it would be responsible to ask implementations to change yet again for something like this.