[performance] Remove hasOwnProperty calls to objects that are managed within React

[mridgway]

Using profiling hasOwnProperty goes from 3.7% of ticks to 0.6% of ticks when calling renderToStaticMarkup 10,000 times. I made sure to only remove calls when the objects are internally created and managed and guaranteed to never have prototypes.

I've also been thinking about optimizations for props. By ensuring that props are a prototype-free object early on in React.createElement a lot of the hasOwnProperty checks downstream could be optimized. I think this may get in to micro-optimization territory though since I'm only seeing 0.6% of ticks in hasOwnProperty at this point (although paths other than renderToStaticMarkup may have more usage like the reconciler).

[facebook-github-bot]

@mridgway updated the pull request.

[facebook-github-bot]

@mridgway updated the pull request.

[mridgway]

The failing test was added in af7e432 but doesn't seem to be testing the right thing. In this case the hasOwnProperty check should be unnecessary since it's a constant.

[sophiebits]

In what engine? Conveniently, I just put up some benchmarking scripts earlier today in #5503. I just ran them and this seems ~1–3% faster in jsc (depending on exactly which number you look at) but slower in node. Not sure why.

In any event, this is a behavior change because we had intended to not crash on inputs like <div valueOf={...} />. I think this also prevents you from passing props that coincide with Object.prototype names because the loop in ReactElement will no longer copy them.

[syranide]

I think this also prevents you from passing props that coincide with Object.prototype names because the loop in ReactElement will no longer copy them.

Primarily, IE8 doesn't have a concept of non-enumerable keys, so all keys are iterated. So to support IE8 you must use this in for-loops.

Having keys with names such as valueOf shouldn't be a problem?

The only other problem is broken setups where someone has done Object.prototype.foobar = 1, foobar is enumerable and will appear as present on all objects. However, I'm not sure if this is actually a problem, I'd expect a lot of problems with various modules if you've done this, and you really really should never ever do it.

[sophiebits]

@syranide I'm having trouble parsing your comment. What I meant was

var config = {toString: 'yolo'};
var props = {};
for (var k in config) {
  if (!RESERVED_PROPS[k]) props[k] = config[k];
}

will not copy toString from config to props (unless I'm missing something).

Like most libraries (e.g., all versions of jQuery), we don't support adding keys to Object.prototype.

[mridgway]

I specifically did not remove the config.hasOwnProperty(propName) check in ReactElement. I only removed the hasOwnProperty call on the RESERVED_PROPS object which is static. I'm confused at how this could have changed the behavior of props since I avoided removing any props related hasOwnProperty except when called on a static hash.

[sophiebits]

RESERVED_PROPS['toString'] is truthy while RESERVED_PROPS.hasOwnProperty('toString') is falsy.

[mridgway]

Ah, got it.

[mridgway]

This discussion came up in another PR I submitted as well: #3665. Usage of Object.create(null) for the constants sounds interesting.

[sophiebits]

How did you benchmark?

[mridgway]

I haven't run benchmarks yet. I was using the v8 profiler to identify top CPU usage. Working on getting my old benchmark.js suite working with React 14 now.

[bloodyowl]

this might be dangerous with IE8, particularly if methods are added to Object.prototype by old MooTools or Prototype versions, which wouldn't be in a defined RESERVED_PROPS. the objects might be for internal use only, but they inherit from the same prototype.