#599 enhancement
Robert Kieffer (broofa)

Performance improvements to function.js + "unwrap()" support

Reported by Robert Kieffer (broofa) | March 17th, 2009 @ 03:50 PM | in 1.7

This is a followup to this (loooong) thread:

http://groups.google.com/group/p...

The attached patch updates the various methods in function.js to use the argument marchalling optimization discussed in the above thread. It also adds support for attaching a "_wrappedFunction" reference that is used by the (new) unwrap() method to provide access to the original function.

Comments and changes to this ticket

  • Juriy Zaytsev

    Juriy Zaytsev March 17th, 2009 @ 04:06 PM

    • Milestone set to 1.7
    • State changed from “new” to “enhancement”
    • Title changed from “[PATCH][TEST] Performance improvements to function.js + "unwrap()" support” to “Performance improvements to function.js + "unwrap()" support”
  • Juriy Zaytsev

    Juriy Zaytsev March 17th, 2009 @ 04:24 PM

    Let's not mix perf. optimization with addition of unwrap. I, for one, would want to see its use case, before polluting Function.prototype with yet another method.

    Also, l0 and l1 are really unfortunate names. It took me few seconds to figure out that there's actually no assignment of 10 to f : )

    
    var f = l0 ? function(){
      ...
    } : function(){
      ...
    }
    

    Something like this would be more clear:

    
    if (boundArgsLen) {
      f = function(){ ... }
    }
    else {
      f = function(){ ... }
    }
    

    Also, how come there's no #1/#2 optimizations for anything other than bind?

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 04:25 PM

    Thanks kangax.

    I'm uneasy about the unwrap support because it's not part of ES 3.1.

    There's a important security reason for that, you might want to pass around a bound function while not allowing access to the unbound one.

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 04:38 PM

    Oops... thanks Robert!

    Also, what's the cost of doing:

    
    function bind(context) {
      if (arguments.length < 2 && Object.isUndefined(context)) return this;
      var __method = this, args = slice.call(arguments, 1), l0 = args.length;
      var f = l0 ? function() {
          args.length = l0;
          update(args, arguments);
          return __method.apply(context, args);
        } : function() {
          return arguments.length
            ? __method.apply(context, arguments)
            : __method.call(context);
        };
      f._wrappedFunction = this;
      return f;
    }
    
  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 05:25 PM

    New patch attached.

    Tobie, Good point about the security issues with unwrap(). Hadn't occured to me. I've removed unwrap() support ( damn it! ;-) )

    kangax, Re: handling case #1/#2 via apply .vs. switch, I think the only place it was missing was in curry(), right? All the other methods (where it might apply) are guaranteed to have non-zero-length args. I've fixed curry by refactoring the bind()/curry() code so curry just leverages the bind() implementation. I'm not sure what you two will think of the code though... let me know. :)

    [BTW, it looks like this change adds ~120 bytes to the final size of prototype.js when gzip'ed]

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 05:27 PM

    kangax: regarding cost of update(), I believe i addressed that in http://groups.google.com/group/p...

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 05:38 PM

    Could you possibly try it without your modified implementation, just resetting the args array like in the example above?

    Perf costs might be due to variable look-ups rather than function call.

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 05:42 PM

    whups, meant that last comment for Tobie.

    Also, forgot to address kangax's comment about l0/l1 naming. Uhm... yeah. The fact the "l" looks like a one (1) doesn't help either, does it? :P

    We could do "len0" and "len1" I guess. I don't have strong opinions, other than it seemed like a good idea to keep the names short since this particular pattern was likely to be appear in more than one spot.

  • Juriy Zaytsev

    Juriy Zaytsev March 17th, 2009 @ 06:00 PM

    @broofa

    #delay can curry arguments as well, so it could benefit from optimization.

    It currently looks like:

    
    function delay(timeout) { 
        var __method = this, args = slice.call(arguments, 1);
        timeout = timeout * 1000
        return window.setTimeout(function() {
          return __method.apply(__method, args);
        }, timeout);
      }
    

    And could be optimized easily like so:

    
    function delay(timeout) {
        var fnOrig = this, 
            fnBound,
            args = slice.call(arguments, 1);
            
        timeout = timeout * 1000
        
        if (args.length) {
          fnBound = function() {
            return fnOrig.apply(fnOrig, args);
          }
        }
        else {
          fnBound = function() {
            return fnOrig.call(fnOrig);
          }
        }
        return window.setTimeout(fnBound, timeout);
      }
    

    Even better, since I don't see a reason to call receiver function in a context of itself (why do we even do it in the first place?), simply alias fnOrig to fnBound:

    
    function delay(timeout) {
      var fnOrig = this,
          args = slice.call(arguments, 1);
          
      timeout = timeout * 1000;
      
      var fnBound = args.length
        ? function(){ return fnOrig.apply(fnOrig, args) }
        : fnOrig
        
      return window.setTimeout(fnBound, timeout);
    }
    

    This way, if you don't pass arguments when calling delay there's no unnecessary burden of creating a wrapper function only to invoke slow apply on original function with an empty array. fn.delay(1) gets practically equivalent to setTimeout(fn, 1000).

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 06:25 PM

    Okay, updated the performance test page. http://www.broofa.com/Tools/JSLi... It now has the following tests: * "Kangax" - As before * "Patch" - Version from last patch I submitted (*) * "custom update()" - Was "Less Improved". Uses a custom update() implementation. * "trunk's update()" - This is your implementation, Tobie. Uses current trunk version of update().

    (*) I removed the "useThis" support from the patch version because it has an adverse affect on performance (due to the extra closure var reference?)

    Here's the results I get: * IE: http://tinyurl.com/dxes2h * FF: http://tinyurl.com/dhe5cd * Safari: http://tinyurl.com/cul9v3 * Opera: http://tinyurl.com/cpylno * Chrome: http://tinyurl.com/cw4wrv

    It looks like both Firefox and IE suffer a bit from the extra method call. Especially Firefox.

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 07:02 PM

    Even better, since I don't see a reason to call receiver function in a context of itself (why do we even do it in the first place?)

    Check with Andrew, but if we don't bind it, scope will fall to the global scope, so that's a backward compatibility issue.

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 07:05 PM

    • Title changed from “Performance improvements to function.js + "unwrap()" support” to “Performance improvements to function.js”

    Re: delay(), I don't really see the point in having fnOrig as the context either. Not only is it rather arbitrary, it's inconsistent with what happens if you pass a function directly to setTimeout(). So I'd suggest the following for delay():

    
    
    function delay(timeout) {
      var __method = this, args = slice.call(arguments, 1);
                
      return window.setTimeout(args.length ?
        function(){ return __method.apply(null, args) } : __method,
        timeout*1000);
    }
    
    

    'nuther patch file attached. (only 66 byte delta to prototype.js.gz this time :) )

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 07:06 PM

    • Title changed from “Performance improvements to function.js” to “Performance improvements to function.js + "unwrap()" support”

    We could do "len0" and "len1" I guess. I don't have strong opinions, other than it seemed like a good idea to keep the names short since this particular pattern was likely to be appear in more than one spot.

    We have a habit of using meaningful names in Prototype. maybe: argLength and curriedArgLength

  • Juriy Zaytsev

    Juriy Zaytsev March 17th, 2009 @ 07:17 PM

    Check with Andrew, but if we don't bind it, scope will fall to the global scope, so that's a backward compatibility issue.

    Of course. This actually looks like an explicit measure to prevent accidental global pollution - bind function to itself rather than to a global scope :)

    While it is a back-compat issue, I think it's very unlikely that someone is relying on context being that of a function object. I'm pretty sure people either work with already bound function, or do not rely on this in the first place.

    Tobie, what do you think about delay change?

    Side effect, of course, is that function will be called in a global context (just like setTimeout does), but we save on an extra closure by removing useless invocation with an empty array.

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 07:21 PM

    Tobie, what do you think about delay change?

    I'd like to have Andrew's opinion.

  • Tobie Langel

    Tobie Langel March 17th, 2009 @ 07:22 PM

    FWIW, I don't think performance issues are really an issue on delayed calls.

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 17th, 2009 @ 07:28 PM

    FWIW, I don't think performance issues are really an issue on delayed calls.

    Agreed. delay() is introducing a 0.01sec delay. I was gonna say something about this before but held off in the interest of getting a patch together that made you folks happy.

    On a different note, is there a reason "method" is the only var prefixed with ""? Seems like it should be just, "method".

  • Juriy Zaytsev

    Juriy Zaytsev March 17th, 2009 @ 07:32 PM

    @broofa

    I would go with fn; method (in a sense of a property of an object) is not even correct at times.

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 19th, 2009 @ 01:05 PM

    So where are we at with this? Is another patch needed? If so, with what changes?

    Kangax, you were going to talk to Andrew about the delay() context, right?

  • Juriy Zaytsev

    Juriy Zaytsev March 19th, 2009 @ 03:58 PM

    • Tag set to patched, tested

    @broofa

    Last patch LGTM. The only thing I would add is a short comment about your optimization - avoidance of array creation - somewhere in the beginning of a functional "module".

    Other than that, we're good to go :)

    Tobie, Andrew?

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) March 19th, 2009 @ 04:33 PM

    Removed first two patch files since they've been superseded.

    Added a patch file that's the same as the other one, but with "_method" renamed to "fn". Take your pick (I prefer the 'fn' name, fwiw)

  • Andrew Dupont

    Andrew Dupont March 19th, 2009 @ 04:54 PM

    I doubt anyone is relying on the "default" scope of Function#delay, so I'm fine with that change.

  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) April 12th, 2009 @ 03:11 PM

    'Hey, just noticed that these changes don't seem to be in the latest 1.6.1 RC (or git repo). Are these still planned for 1.6.1?

  • Tobie Langel

    Tobie Langel July 23rd, 2009 @ 03:59 AM

    • Tag changed from patched, tested to patched, perf, tested
  • Tobie Langel

    Tobie Langel July 23rd, 2009 @ 04:05 AM

    • Tag changed from patched, perf, tested to patched, performance, tested
  • Robert Kieffer (broofa)

    Robert Kieffer (broofa) September 6th, 2009 @ 04:09 PM

    • Tag changed from patched, performance, tested to patched, performance

    Revised patch to include inline documentation, and descriptive var names, as per this thread:

    http://groups.google.com/group/prototype-core/browse_thread/thread/...

  • T.J. Crowder

    T.J. Crowder November 16th, 2009 @ 04:50 PM

    • Assigned user cleared.

    [responsible:none bulk edit command]

  • Tobie Langel

    Tobie Langel March 1st, 2010 @ 01:25 AM

    • Tag changed from patched, performance to patched, performance, section:lang
  • Jason Westbrook
  • chenlina1
  • julian

    julian November 10th, 2017 @ 06:11 AM

    Great post buddy. You are doing a great job.
    https://msofficesupportnumber.org/

  • Lyrics Songs

    Lyrics Songs November 15th, 2017 @ 07:52 PM

    I really like this post.it is very useful knowledgeable.
    Lyrics Songs
    Guitar Chords

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Attachments

Referenced by

Pages