search

davidmarkclements / rfdc

Really Fast Deep Clone
MIT License
643 stars 25 forks source link

Support Symbol properties #12

Open silverwind opened 5 years ago

silverwind commented 5 years ago

Use case would be cloning a React component. If it hurts performance too much, I suggest making it opt-in.

BridgeAR commented 5 years ago

Sure, PR welcome. It would have to use Object.getOwnPropertySymbols() and filter all non-enumerable symbols when the option is set.

It won't hurt the performance in case we duplicate all functions. That does not sound like a good option though.

I would benchmark the difference in case we combine clone and cloneProto and just check for the proto and a symbolProps option inside of the new clone function compared to the current implementation. Ideally, V8 is able to detect that the option (when always run with the same option) is constant and optimizes the function with dead code elimination.

silverwind commented 5 years ago

Did some experiments at https://github.com/silverwind/rfdc/commit/9eeca0320c48e51c7f325bdb669adbfdc0714603, and I see a 30% performance regression in the benchmark. So we would need to get clever.

That dead code elimination is something to try, but I would make sure it also works in other JS engines, ideally also adding engines like Spidermonkey to the benchmarking.

silverwind commented 5 years ago

Maybe symbol properties are not really needed to clone a React component after all. They do use symbols, but only as values and those are already cloned fine currently.

I don't have a use-case for cloning with symbols as keys, so I guess I won't chase this further. Anyone who wants to should make sure it either does not regress performance significantly and if it does, make it optional.

BridgeAR commented 5 years ago

Always checking for the symbol properties will definitely hurt performance. It should only be an opt-in, if added at all. Adding another option should not show a significant performance difference in case it is deactivated.

BridgeAR commented 4 years ago

@silverwind would you still like to open a PR to add this feature behind a new option?

silverwind commented 4 years ago

I don't have a need for this currently. With the way this module is layed out, it would probably require significant code duplication, e.g. rfdcCircles, rfdcSymbols and rfdcCirclesSymbols which also put me off.