Begin replacing propertyInfo.* access with functions #11733

gaearon · 2017-12-01T00:40:05Z

The goal is to remove the "injection" step at initialization. I find it confusing because we always have to look in three places: the configs, the initialization logic, and then the use of those properties. I think this is partly why the logic got so tangled by now.

I'm starting by removing the propertyInfo.* properties one by one, replacing them with function calls. There might be better implementations—happy to revise them. In some cases it may seem like we'd have to repeat an attribute name a few more times than before, but I think it probably gzips well, and in any case I'll make sure the size is smaller or the same before merging.

Note I'm not changing logic yet. There are a few oddities in how code paths differ based on whether propertyInfo exists—which ideally shouldn't be the case. I'll looking at that later.

Not sure why we did this. Don't need now.

nhunzaker · 2017-12-01T00:52:30Z

packages/react-dom/src/shared/DOMProperty.js

-    // different attribute names (see DOMAttributeNames below)
+    // different attribute names (see `attributeNames`)
+    // TODO: this doesn't seem great? Probably means we depend on
+    // existence in the whitelist somewhere we shouldn't need to.


I remember when we hit this adding custom attributes, and it didn't feel great. It would be awesome to get to the bottom of this.

Yea, definitely will. I have another set of commits that dives into this but it turned out to be a rabbit hole and I decided to do these first.

nhunzaker · 2017-12-01T00:56:54Z

packages/react-dom/src/shared/DOMProperty.js

-    multiple: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
-    muted: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
+    multiple: HAS_BOOLEAN_VALUE,
+    muted: HAS_BOOLEAN_VALUE,


I think this is probably fine, but why the change?

Not sure what you mean? I removed the flag.
c9ca797
Easier to read as individual commits.

Got it. Thank you!

@gaearon at this point it looks like there aren't any properties that contain more than one flag. We could probably stop using bitmasks and just use an enum, remove checkMask and use a single property in propertyInfo instead of 6.

My plan is to get rid of propertyInfo completely (maybe). This PR is just very iterative.

This is a good observation though.

nhunzaker · 2017-12-01T01:03:00Z

packages/react-dom/src/client/DOMPropertyOperations.js

-      var attributeName = propertyInfo.attributeName;
-      var namespace = propertyInfo.attributeNamespace;
+      var attributeName = getAttributeName(name);
+      var namespace = getAttributeNamespace(name);
      // `setAttribute` with objects becomes only `[object]` in IE8/9,
      // ('' + value) makes it output the correct toString()-value.


Is this true in IE9?

Also valueOf:

Maybe only IE8?

(Regardless, don't want to change the logic here, only the structure :-)

Totally. It's just nice to look at this code again with a fresh set of eyes. I'll file an issue.

aweary · 2017-12-01T01:09:08Z

packages/react-dom/src/shared/DOMProperty.js

+
+export function shouldUseProperty(name) {
+  return usePropertiesFor.has(name);
+}


Could this just be inlined in DOMPropertyOperations? Doesn't look like it's used anywhere else. Also, we could then just do check the set directly instead of a function call too.

I want to consolidate all property/attribute stuff here first, then refactor it to make function contracts clearer, then possibly spread it out. I'd rather not spread it out yet.

It's just easier for me to deal with it if it's all in one file. Easier to see patterns.

nhunzaker · 2017-12-01T01:10:28Z

packages/react-dom/src/client/DOMPropertyOperations.js

-      var attributeName = propertyInfo.attributeName;
-      var namespace = propertyInfo.attributeNamespace;
+      var attributeName = getAttributeName(name);
+      var namespace = getAttributeNamespace(name);
      // `setAttribute` with objects becomes only `[object]` in IE8/9,
      // ('' + value) makes it output the correct toString()-value.


Internally they're still implemented as flags. But this is now an implementation detail I can change.

Now that all propertyInfo access is internal to DOMProperty, we can stop exposing the property info. This shows the code paths that are different depending on whether something is whitelisted or not. These are bad because ideally our other helper functions should work just fine regardless of the whitelist (they just should have correct defaults for things that aren't in the list). We will need to start getting rid of these calls.

This removes the existing duplication.

My previous changed caused it to access the fields multiple times. Now that it's encapsulated inside DOMProperty, I can implement it efficiently again.

Instead, keep a Map of name to type constant. The only real mask at this point was positive numeric + numeric, so I used switch fallthrough.

gaearon · 2017-12-01T02:12:04Z

@aweary Indeed it's much nicer without masks, thanks for the tip.

nhunzaker · 2017-12-01T02:13:47Z

packages/react-dom/src/shared/DOMProperty.js

+// An attribute that can be used as a flag as well as with a value.
+// Removed when strictly equal to false; present without a value when
+// strictly equal to true; present with a value otherwise.
+const OVERLOADED_BOOLEAN = 5;


aweary · 2017-12-01T17:35:50Z

@gaearon glad it worked out, it looks great!

At this point, I wonder if we could just get rid of HTMLDOMPropertyConfig and initialize the propertyTypes map with those values? Then we can get rid of injectDOMPropertyConfig in favor of something SVG-specific since those are the only values that are dynamically generated at runtime.

We'd lose out on the injectDOMPropertyConfig warning, but I'm not sure if it has a lot of value anyways since we're not exporting injection logic right?

gaearon · 2017-12-01T18:40:01Z

At this point, I wonder if we could just get rid of HTMLDOMPropertyConfig and initialize the propertyTypes map with those values?

Yes but we'll need to see what's more compact in terms of bundle size. I'm not sure.

aweary · 2017-12-01T19:03:59Z

Yes but we'll need to see what's more compact in terms of bundle size. I'm not sure.

@gaearon I checked this out locally and made the change to compare bundle sizes.

By inlining the config the NODE_PROD build for react-dom.production.min.js went from 29.39kb to 29.34kb, and the UMD build went from 30.49kb to 30.46kb. So inlining will also reduce the bundle size a little!

gaearon · 2017-12-01T19:16:21Z

Nice. Please push to my branch?

These changes seem to make the results neutral again (after a small regression).

nhunzaker · 2017-12-01T23:08:34Z

packages/react-dom/src/shared/DOMProperty.js

-      propertyInfo.hasOverloadedBooleanValue
-    );
+  if (propertyTypes.has(name)) {
+    switch (propertyTypes.get(name)) {


Is the overhead of the switch greater than the double lookup of has and get?

I was trying to avoid making it polymorphic (number/undefined). Maybe that's silly, dunno.

nhunzaker · 2017-12-01T23:14:39Z

packages/react-dom/src/shared/DOMProperty.js

+  'multiple',
+  'muted',
+  'selected',
+]);


I know microbenchmarks are dangerous, but would indexOf or object property membership be faster?

https://esbench.com/bench/5a21e22199634800a03494ec

I bet in the long-term checking set membership will be faster, since that's the sole purpose of the data structure. If we're still getting 25 million ops/s with a set it's probably fine to use it since its the correct data structure for this.

Ya. I hesitated to say anything. Dan just mentioned the extra look-ups were slowing things down a bit.

Feel free to disregard.

If it's a non-trivial performance regression we should definitely consider changing it. It's just so nice using Set and Map instead of arrays and objects 😍

gaearon · 2017-12-02T14:42:46Z

Meh. It's too slow. I need to rethink this.

gaearon · 2017-12-10T16:59:39Z

#11815

gaearon added 5 commits December 1, 2017 00:08

Replace .attributeNamespace with a method

e8d8042

Remove .propertyName

dcef72c

Replace .attributeName with getAttributeName()

8366004

Replace MUST_USE_PROPERTY with shouldUseProperty()

c9ca797

Stop exporting properties from DOMProperty

0c067a0

Not sure why we did this. Don't need now.

gaearon force-pushed the dom-inline-more branch from 06037ea to 0c067a0 Compare December 1, 2017 00:42

nhunzaker reviewed Dec 1, 2017

View reviewed changes

aweary reviewed Dec 1, 2017

View reviewed changes

nhunzaker approved these changes Dec 1, 2017

View reviewed changes

gaearon added 2 commits December 1, 2017 01:17

Replace .has*Value flags with functions

bf74f67

Internally they're still implemented as flags. But this is now an implementation detail I can change.

gaearon force-pushed the dom-inline-more branch from 8479472 to 2423bd6 Compare December 1, 2017 01:24

Move shouldIgnoreValue() into DOMProperty

1336ef3

This removes the existing duplication.

gaearon force-pushed the dom-inline-more branch from 9e29763 to 495297f Compare December 1, 2017 01:31

gaearon added 2 commits December 1, 2017 01:41

Make shouldIgnoreValue() more efficient again

240d071

My previous changed caused it to access the fields multiple times. Now that it's encapsulated inside DOMProperty, I can implement it efficiently again.

Remove old comment

650d731

gaearon force-pushed the dom-inline-more branch from c4a6ddd to 650d731 Compare December 1, 2017 01:41

facebook-github-bot added the CLA Signed label Dec 1, 2017

Remove masking completely

e5f2e0a

Instead, keep a Map of name to type constant. The only real mask at this point was positive numeric + numeric, so I used switch fallthrough.

nhunzaker reviewed Dec 1, 2017

View reviewed changes

Flowify DOMProperty

e0a6d44

Inline property configs in propertyTypes map

63703c2

aweary force-pushed the dom-inline-more branch from 209ff28 to 63703c2 Compare December 1, 2017 19:29

gaearon added 3 commits December 1, 2017 19:33

Merge branch 'master' into dom-inline-more

d4495a0

Merge branch 'master' into dom-inline-more

82a0d09

Minor tweaks

c236b23

These changes seem to make the results neutral again (after a small regression).

nhunzaker reviewed Dec 1, 2017

View reviewed changes

gaearon closed this Dec 2, 2017

gaearon mentioned this pull request Dec 10, 2017

Refactor DOM attribute code (take two) #11815

Merged

gaearon mentioned this pull request Apr 23, 2018

Use browser event names for top-level event types in React DOM #12629

Merged

Begin replacing propertyInfo.* access with functions #11733

Begin replacing propertyInfo.* access with functions #11733

Uh oh!

Conversation

gaearon commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhunzaker Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweary commented Dec 1, 2017

Uh oh!

gaearon commented Dec 1, 2017

Uh oh!

aweary commented Dec 1, 2017

Uh oh!

gaearon commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhunzaker Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 2, 2017

Uh oh!

gaearon commented Dec 10, 2017

Uh oh!

Uh oh!

gaearon commented Dec 1, 2017 •

edited

Loading

gaearon Dec 1, 2017 •

edited

Loading

nhunzaker Dec 1, 2017 •

edited

Loading

nhunzaker Dec 1, 2017 •

edited

Loading