Use Rest Parameters where it makes sense #637

Merged
merged 1 commit into from Dec 12, 2013

Conversation

Projects
None yet
9 participants
Member

cpojer commented Dec 5, 2013

Depends on #636

This changes a bunch functions to use rest parameters instead of 'arguments'.

It makes the code simpler to read and ensures we always work with arrays instead of an imposter.

Member

cpojer commented Dec 5, 2013

The first commit should go away after it was pulled, right?

Member

jordwalke commented Dec 5, 2013

Could you run performance tests on this (especially since the actual use of arguments is out of our control)?

Member

cpojer commented Dec 5, 2013

Yes but how? :)

Owner

zpao commented Dec 5, 2013

Yea, that first commit will go away. http://paulshen.github.io/react-bench/ has some benchmarks.

Also, let's turn on the esnext option for jshint so lint passes

Member

jeffmo commented Dec 5, 2013

@jordwalke: This uses arguments under-the-hood anyway, it's mostly just sugar for doing so at this point...

Member

cpojer commented Dec 5, 2013

it is really just Array.prototype.slice.call(arguments, offsetA, offsetB).

Member

jordwalke commented Dec 5, 2013

@jeffmo: I've seen diffs far less concerning than this cause huge performance regressions. This diff touches the single most frequently invoked method in the entire React core (the constructor for all components). No matter how innocuous seeming, I always do a perf test for any diff that touches the critical path.

Member

jeffmo commented Dec 5, 2013

Gotcha, fair enough. Question remains about how to do so though, I think?

Member

jeffmo commented Dec 5, 2013

(also somewhat surprised that we're using arguments in our single most frequently invoked method in the entire React core)

Member

sebmarkbage commented Dec 5, 2013

Believe me the performance of that was well contested when people trusted micro-benchmarks more than real world usage. I'd be very hesitant to mess with that optimization. The next bottleneck is getting the multi-var arguments from the convenience constructor into here. Potentially through another constructor function. Thankfully modern engines will help us there.

Member

jordwalke commented Dec 5, 2013

@sebmarkbage: Certain uses of arguments are problematic, (not all) but I'm not as much concerned with that here - I'm more concerned about the fact that this is a diff that touches the most critical path. I have no other basis for concern :)

@spicyj spicyj and 1 other commented on an outdated diff Dec 5, 2013

src/core/ReactComponent.js
@@ -301,22 +301,13 @@ var ReactComponent = {
// this._owner !== this._pendingOwner.
this._pendingOwner = this._owner;
- // Children can be more than one argument
- var childrenLength = arguments.length - 1;
- if (childrenLength === 1) {
- if (__DEV__) {
- validateChildKeys(children);
- }
+ if (__DEV__) {
+ children.forEach(validateChildKeys);
+ }
+ if (children.length === 1) {
+ this.props.children = children[0];
@spicyj

spicyj Dec 5, 2013

Member

You may also want to benchmark just storing this as children always. The original motivation was to avoid the array allocation but if you're going to always have it it might not make sense to bother with the unwrapping.

@sebmarkbage

sebmarkbage Dec 5, 2013

Member

We actually have quite a bit of code that depends on it being a single child. :/

@spicyj

spicyj Dec 5, 2013

Member

Ahh right, forgot about composite components looking at children.

@spicyj spicyj commented on an outdated diff Dec 5, 2013

src/vendor/error/ex.js
@@ -29,10 +29,8 @@
* @param {string} errorMessage
*/
-var ex = function(errorMessage/*, arg1, arg2, ...*/) {
- var args = Array.prototype.slice.call(arguments).map(function(arg) {
- return String(arg);
- });
+var ex = function(errorMessage, ...args) {
+ args = args.map((arg) => String(arg));
@spicyj

spicyj Dec 5, 2013

Member

wow, arrow functions are so pretty.

Member

jordwalke commented Dec 5, 2013

it is really just Array.prototype.slice.call(arguments, offsetA, offsetB).

So we create an additional array every time the method is invoked? Regardless of what the benchmarks say, maybe we should hold off on rest args for ReactComponent's constructor. That little function is likely invoked more than any other function in React. Also, a large portion of instances have only one child, and therefore would avoid the array allocation.

Member

jeffmo commented Dec 5, 2013

We really need a higher-level metric to shoot for here. Gut-feeling on whether "another allocation" is ok or not is only going to lead to more subjective debate and potential for mis-guided optimization effort. I think perf questions are valid (including here) -- as long as we have a concrete way to answer them. I'm a little worried that we're heading towards a tangential meta-discussion right now.

@cpojer asked earlier what the best way to perf-test this would be -- do we have an answer for this? If not, we're not doing ourselves any favors in speculating here and leaving red-tape around "sensitive" core functions.

Member

cpojer commented Dec 5, 2013

  • Reverted the changes to the base constructor (I did however clean it up just a little for DEV)
  • Made joinClasses and ex smaller and easier to understand
  • Added esnext option to to .jshintrc Note that this will fail for now until jshint/jshint#1386 gets pull, @zpao agreed that this is ok
  • I benchmarked this using @paulshen's tool. I didn't get any significant difference between master and this diff (I re-ran a couple of times, sometimes master was faster and sometimes this diff was faster)

Pull away!

Member

jeffmo commented Dec 5, 2013

Code looks good -- I'll leave it to @jordwalke to merge in case he has any further concerns

@subtleGradient subtleGradient and 4 others commented on an outdated diff Dec 5, 2013

src/utils/joinClasses.js
@@ -26,19 +26,8 @@
* @param {...?string} classes
* @return {string}
*/
-function joinClasses(className/*, ... */) {
- if (!className) {
- className = '';
- }
- var nextClass;
- var argLength = arguments.length;
- if (argLength > 1) {
- for (var ii = 1; ii < argLength; ii++) {
- nextClass = arguments[ii];
- nextClass && (className += ' ' + nextClass);
- }
- }
- return className;
+function joinClasses(...classes) {
+ return classes.filter((className) => !!className).join(' ');
@subtleGradient

subtleGradient Dec 5, 2013

Contributor

Have you done any perf comparisons?
I'm not completely sure, but it looks like you're creating a new function here and then creating an unnecessary array just so that you can turn it into a string. Pretty? sure. But I don't think that's really worth the cost

@jeffmo

jeffmo Dec 5, 2013

Member

Amdahl's law, etc

@andreypopp

andreypopp Dec 5, 2013

Contributor

return classes.filter(Boolean).join(' ') ?

@andreypopp

andreypopp Dec 5, 2013

Contributor

I hope escape analysis (does it already work in modern js engines?) would help to eliminate array allocation...

@cpojer

cpojer Dec 5, 2013

Member

I think it is fine.

As said, I ran react-perf and it didn't become slower than before.

@zpao

zpao Dec 5, 2013

Owner

The jsperf linked in the docblock has very different results for using array vs args (also different across browsers...).

@cpojer

cpojer Dec 6, 2013

Member

I'm super unhappy but I perf-tested this and it was about 94 times slower. Not awesome. JS engines, get your shit together. Anyway, I just reverted the change here for until 2020 when at least one browser will have ES6 natively.

@sebmarkbage sebmarkbage commented on an outdated diff Dec 6, 2013

src/core/ReactComponent.js
@@ -302,18 +302,16 @@ var ReactComponent = {
this._pendingOwner = this._owner;
// Children can be more than one argument
+ if (__DEV__) {
+ Array.prototype.slice.call(arguments, 1).forEach(validateChildKeys);
@sebmarkbage

sebmarkbage Dec 6, 2013

Member

Not sure how this is cleaner? The prototype access + call is one of the ugliest parts of JS and very esoteric looking.

Please perf test this too. @jordwalke found that this key validation step was actually a huge performance bottle neck at one point in DEV and unfortunately people perf test DEV builds too. Makes us look bad.

Member

sebmarkbage commented Dec 6, 2013

@cpojer What's the perf results in IE8?

I agree that we don't have enough good tests and tools to do perf testing reliably right now. That means that we're often best suited to use our own intuition atm. That means we shouldn't block on potentially misguided optimization efforts.

However, this code has been through a lot of testing in individual products which have been fine tuned. This was not found to be a bottle neck. Changing this code path may or may not cause a regression in those products. Something we may not discover much later.

IMO, it needs to be a clear win for this refactoring to take the place of the tested code. I.e. a new API surface or clear readability win. IMHO, this is mostly subjective and unnecessary risk.

Member

cpojer commented Dec 6, 2013

One important reason is that arguments are not really inferable statically, while rest params are. @jeffmo will fill you in. Ideally after ES6 becomes a standard, arguments will just cease to exist.

Member

sebmarkbage commented Dec 6, 2013

If this was new code I wouldn't have a problem with it. Since neither had been tested and we'd just be speculating.

@cpojer that would've been good to have in the initial description. If this is something we imminently need for static analysis. Let's continue that offline.

Member

cpojer commented Dec 11, 2013

Alright, just rebased this and reverted the changes to ReactComponent for now so that this change can be pulled.

Member

jeffmo commented Dec 12, 2013

We know that this is harmless with equal confidence that we know it's harmful.
We have eyeballs and users and smart engineers that can make changes at any time.
And somehow we're still scared to do something that, for all we know, is a win.

Yay ignorance-induced red tape!

benjamn merged commit 7c95194 into facebook:master Dec 12, 2013

1 check passed

default The Travis CI build passed
Details

cpojer deleted the cpojer:use-rest-params branch Dec 27, 2013

Contributor

subtleGradient commented Jan 6, 2014

Perf comparison of this diff to previous master in IE8.
screen shot 2014-01-06 at 1 06 08 pm

Contributor

subtleGradient commented Jan 6, 2014

IE11 on a Windows Surface Pro 1 running Windows 8.1

1556311_10151816339281175_448734076_o

Contributor

subtleGradient commented Jan 6, 2014

IE9 running on a crappy Windows 7 Starter Edition netbook.

886182_10151816336596175_1621545644_o

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment