Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for an ordered attribute white list, and new property injection types #7428

Closed
nhunzaker opened this issue Aug 4, 2016 · 9 comments

Comments

@nhunzaker
Copy link
Contributor

nhunzaker commented Aug 4, 2016

I've been working a lot with the ReactDOMInput wrapper to fix IE and Chrome bugs and I've noticed that a good portion of the code exists to prevent attributes from being assigned in the wrong order, and for inserting them in a specific way.

So that got me curious about benefits centralized, ordered, property assignment might yield. I'm interested in:

  1. Eliminating special attribute assigning ordering cases. Like assigning type and step before value.
  2. Eliminate most special value, defaultValue, checked, and defaultChecked assignment order code.

I've been investigating that by reimplementing my Chrome backspace fix #7359 with more logic moved to DOMProperty. Code from ReactDOMInput can be eliminated because of guarantees in property assignment order. I'm very curious if it could be eliminated from the other wrappers and other DOM utilities.

This work is here: master...nhunzaker:nh-ordered-props. I sort of hesitate to share, but I'm curious if this the type of thing you would accept as a contribution, and is it safe to do this work in parallel with the React Fiber feature?

If so, I was going to start reproducing all of the edge cases fixed by the wrappers, and start working towards reducing the amount of "rule breaking" they require to get properties to assign as expected. Otherwise, I guess I have a much better understanding of how property assignment works :).

@zpao
Copy link
Member

zpao commented Aug 4, 2016

I like the idea of improving this code, however generalizing too much seems like it's likely to introduce perf regressions for every other case (Object.keys(props).sort === array allocation and O(something) operation for every).

I think it's good to explore but we'll probably be a bit more cautious around this code. cc @spicyj since he's probably done the most with this code recently.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 5, 2016

The performance angle of this is a bit of a bummer. I'm killing the cost of the index look up when sorting by storing the order of a property on insertion time, but I haven't figured out a way to get around the cost of building the array every time.

Something for me to mull over.

@syranide
Copy link
Contributor

syranide commented Aug 5, 2016

['accept', 0] is also a no-no. The left-most names must be keys for GCC advanced optimizations to work.

This problem should only extend to inputs and perhaps very few related elements, so it probably doesn't really make sense to do it globally. Had JS been lower-level it may have been negligible but I doubt it will be in JS.

As for the O(nlogn) sort(), that can be solved by implementing radix sorting in this case which would be O(n) (under the right circumstances), you could even avoid additional allocations, but I imagine the actual overhead would still be significant. But in reality, I'm quite sure the everything-considered-solution here is to just implement these as special-cases per element type, the number of affected attributes per element is minimal so simply iterating the special ones in a pre-defined order is the way to go (and skipping them if they aren't set, rather than the reverse of taking the set ones and re-ordering them). EDIT: Or perhaps marking the must-be-ordered props with a flag which separates them out for fast sorting and leaving all other properties with the current approach.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 5, 2016

Implementing this for specific elements (I'm specifically looking at input and eventually textarea) seems like a good compromise middle-ground. I'll jam on for a bit and see what it looks like.

@nhunzaker
Copy link
Contributor Author

As for @syranide's comment about GCC optimization. Bummer... There's only 6 or so properties that need to be ordered. I'll revert my array-ification and just keep a list of those properties. Maybe then I can migrate that to the input wrapper and see what localizing the ordered property list could look like.

@zpao
Copy link
Member

zpao commented Aug 5, 2016

FWIW I actually don't think GCC is that important here. I think these shouldn't be getting crushed, even in advanced mode, since they are mostly DOM properties (might be a couple cases where that isn't true).

@nhunzaker
Copy link
Contributor Author

Slowly chipping away at this. It's a fun problem.

Keeping a tag specific priority list solves the performance problem. Basically: check to see if the tag name is defined within a map of priorities. If so, then enumerate over those properties first.

Benchmarks report about what you'd expect: controlled inputs are faster because they do less work. Everything else appears to be roughly the same (or at least within the margin of error).

@nhunzaker
Copy link
Contributor Author

I finally got this to a state that I feel good about. I thought it might be easier just to comment on the code directly:

#7474

@nhunzaker
Copy link
Contributor Author

Closing this one out. I might revisit this after some progress is made on removing the attribute whitelist, but that might be a ways off. Thanks for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants