Copying arguments

[T.J. Crowder]

Hi all,

In the 1.6.1 source, we're grabbing a reference to Array's `slice`
method and then using that in a variety of places to copy subsets (or
sometimes entire sets) of arguments, like so:

var slice = Array.prototype.slice;
//. ...
function bind(context) {
if (arguments.length < 2 && Object.isUndefined(arguments[0]))
return this;
var __method = this, args = slice.call(arguments, 1);
return function() {
var a = merge(args, arguments);
return __method.apply(context, a);
}
}

This is presumably on the assumption that using the built-in slice
method will perform better than our own loop would. It's a perfectly
reasonable assumption. I *like* the assumption. The assumption makes
*sense* to me.

It just happens to be wrong:
http://pastie.org/605898

(Well, except on Firefox.)

On Chrome, using Array.prototype.slice via Function#call is 8-10 TIMES
slower than our own function would be; on IE, it's about 25% slower,
on Safari and Opera it's about 50% slower. On Firefox, the odd one
out, the "own loop" is 50% slower than `slice`.

I mention this for three reasons.

1. We might consider providing a copy function like the one in that
pastie and using it in the various places we copy args skipping the
first few. It adds code, but improves performance -- except in
Firefox -- in some key areas (binding, currying, etc.), and
performance is an issue.

2. It's a reminder to check our assumptions.

3. It raises a question I've had for a while: How do you "feature
test" performance stuff? This isn't the only thing like this.
There's this question/answer[1] over on stackoverflow, for instance.
Writing my originally-brief answer I made a reasonable assumption
about performance, someone checked it and found that I was Quite Wrong
Thank You, and that lead to my finding wild differences in a very
simple operation (zero-filling an array) across implementations.

FWIW,

[1] http://stackoverflow.com/questions/1295584/most-efficient-way-to-create-a-zero-filled-javascript-array/1295671
--
T.J. Crowder
tj / crowder software / com
www.crowdersoftware.com

[Tobie Langel]

We have a rewrite of function.js waiting to be included. It does
something even smarter thanks to a great idea by Broofa.

We'll add it in as soon as we've handled our pdoc and website issues.

Best,

Tobie

> [1]http://stackoverflow.com/questions/1295584/most-efficient-way-to-crea...

[kangax]

On Sep 4, 7:39 pm, Tobie Langel <tobie.lan...@gmail.com> wrote:
> We have a rewrite of function.js waiting to be included. It does
> something even smarter thanks to a great idea by Broofa.
>
> We'll add it in as soon as we've handled our pdoc and website issues.

That patch looks good to go [1] (I think we all agreed on it in a
ticket discussion, after all the revisions).

Why not commit it now?

[1] https://prototype.lighthouseapp.com/projects/8886/tickets/599-patchtest-performance-improvements-to-functionjs-unwrap-support

[...]

--
kangax

[T.J. Crowder]

I was wondering where that discussion had gone; looking at the 1.6.1
source I thought "Strange, does this do the stuff from the discussion
and I'm missing it?"

@kangax: The patch I'm seeing[1] still uses slice, which we can
probably improve on. Doesn't mean we shouldn't commit the patch and
then try to improve from there, though.

[1] https://prototype.lighthouseapp.com/projects/8886/tickets/599/a/101731/with_method_renamed_to_fn.patch

-- T.J.

On Sep 5, 5:20 am, kangax <kan...@gmail.com> wrote:
> On Sep 4, 7:39 pm, Tobie Langel <tobie.lan...@gmail.com> wrote:
>
> > We have a rewrite of function.js waiting to be included. It does
> > something even smarter thanks to a great idea by Broofa.
>
> > We'll add it in as soon as we've handled our pdoc and website issues.
>
> That patch looks good to go [1] (I think we all agreed on it in a
> ticket discussion, after all the revisions).
>
> Why not commit it now?
>

> [1]https://prototype.lighthouseapp.com/projects/8886/tickets/599-patchte...
>
> [...]
>
> --
> kangax

[Tobie Langel]

Hi, kangax.

I understand and appreciate your eagerness (I feel the same).

However, for the benefit of our users and the code base, I think it's
important that we:

1) handle the most pressing issues at hand first (mainly doc related),
2) agree on stylistic issues (this patch clearly doesn't look very
prototypish to me), and
3) discuss performance versus LOC issues and reformat that patch
accordingly.

Let's focus our attention where it's most needed at the moment:
completing the documentation.

Thanks for your understanding.

Best,

Tobie

[Robert Kieffer]

Tobey, like TJ, I've been eagerly waiting for these changes to appear
and puzzled by the delay. I was under the impression the concerns you
and others had were addressed back in March, and that pushing this
change into the codebase was a little more than a matter of committing
the patch.

To be met with abstract arguments about "stylistic issues" and
"performance .vs. LOC" now is surprising and discouraging. I'm pretty
sure we discussed _exactly_ those issues back in March; I believe I
provided data on how this patch would affect the prototype.js file
size, and we went back and forth over things as trivial as the names
of local variables.

I don't *need* to have this patch make it into the prototype
distribution - as I said in my original post, we've patched the
prototype core, so it's little more than an occasional nuisance when
we upgrade to migrate those changes. But as the main proponent for
this patch, which as an improvement to core performance will benefit
nearly everyone who uses Prototype, I'm at a loss for how to move this
along. And frustrated by that.

Sorry, guess that's not very understanding. :-J

[Tobie Langel]

Robert,

Could you kindly point me towards that patch.

I don't think it's the one kangax mentioned.

Thanks,

Tobie

[Robert Kieffer]

Tobey, I believe it is the same bug that kangax referred to:

https://prototype.lighthouseapp.com/projects/8886/tickets/599-patchtest-performance-improvements-to-functionjs-unwrap-support

The original discussion (linked to in the above ticket) can be found
here:

http://groups.google.com/group/prototype-scriptaculous/browse_thread/thread/1e62f11be350eb1d/53623feabd6319e2?hl=en&

[Tobie Langel]

Hi again, Robert.

The patch linked by kangax certainly doesn't account for the various
things we discussed back then.

It notably doesn't document the reasons why and how your (very smart)
implementation works.

That patch also has various "stylistic" issues which I remember
discussing and that we had all agreed to modify for a final patch.

If I recall correctly, this patch just didn't make it in because a
final, reviewed and cleaned up patch wasn't submitted. An unfortunate
yet frequent issue with OSS.

Given the amount of work that was put in this patch and the huge perf
benefits it brings about, I think it makes sense to add it to an
otherwise frozen 1.6.1 version provided a proper patch gets submitted.

Best,

Tobie

Unrelated P.S.: Would appreciated not seeing my first name
butchered. :-)

[Robert Kieffer]

Whups, sorry about butchering your name. I'm seriously sleep deprived
these days - a result of our new 5-month old kid. :-)

Re: Documentation for how and why the patch works - Is that a new
requirement for Prototype code? This is the first I've heard of
something like this. My original post in the discussion (link above)
goes into a fair bit of detail. Where does this documentation go?
What is expected? Sorry, it's been a while since I looked at the
prototype code base and, as of March, I thought my patch was pretty
consistent with how things are done.

I apologize for being so obtuse on this, but can you please elaborate
on what you mean by "a final, reviewed, and cleaned up patch"?In re-
reading the ticket, I simply don't see where the issues are. The only
issue that I guess didn't get addressed was your request for more
descriptive variable names... but that didn't seem to be a showstopper
(nor consistently applied w/in the existing codebase). But if it
makes a difference, I'm happy to do the renaming.

Cheers,
- rwk

[T.J. Crowder]

Robert,

You're right, this stuff is not clearly laid out anywhere, and I'm
going to try to take on the task of fixing that (*after* documentation
migration) if no one else is yet (I know the core guys are already
aware of that issue and some work has already been done). It can be
very frustrating when you're trying to contribute something to have
stylistic stuff thrown up when the closest thing we have to a
published style guide is four bullet points on the prototypejs.org/
contribute page. :-) And the medium (lighthouse) encouraging
terseness doesn't help on the encouragement front.

It _is_ important for newly contributed code to fit in (which has
previously been a terrible mish-mash and which core are trying to
standardize, with big gains in the 1.6.1 stuff), as I'm sure you
already know. My read of the ticket and patch is that these are the
outstanding requests:

1. Use argLength and origArgLength or similar descriptive argument
names rather than len0, etc.

2. Include a // comment in the implementation describing the why and
how of your cool idea. (// and PDoc comments are stripped by rake
dist, /* ... */ are not, hence making it a // comment -- it's for
other developers).

Both of those are things that have not been done enough in the past
and have held back progress, hence trying to move move toward them.

As a Prototype user, thank you again for your efforts at improving
efficiency, greatly appreciated,

--
T.J. Crowder
tj / crowder software / com
www.crowdersoftware.com