Refactor DOM Bindings Completely Off of DOMProperty Meta Programming #26546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This ensures that we can hard code special cases directly in the switch to get the correct semantics.
What matters is that we treat them as lower case when we read them.
This is the last of the DOMProperty configs.
facebook-github-bot
added
the
React Core Team
sebmarkbage
commented
Apr 4, 2023
| props: any, | ||
| ): void { | ||
| switch (key) { | ||
| case 'style': { |
There was a problem hiding this comment.
This is a replica of the beginning of setProp. We can probably unify some of this.
sophiebits
approved these changes
Apr 4, 2023
| } | ||
| case 'suppressContentEditableWarning': | ||
| case 'suppressHydrationWarning': | ||
| case 'innerHTML': { |
There was a problem hiding this comment.
:o since when is innerHTML reserved
There was a problem hiding this comment.
It's been reserved forever basically. It became more critical with the Custom Element convention which looks for existing properties and use them if available. Since that will be allowing plain innerHTML without using dangerouslySetInnerHTML.
| case 'innerHTML': { | ||
| // Noop | ||
| break; | ||
| } |
There was a problem hiding this comment.
is no longer skipping defaultValue defaultChecked autoFocus and no longer setting multiple muted technically a breaking change?
There was a problem hiding this comment.
Yea but probably fine. Since this path is no longer taken for is etc where this might be relevant in the weird overlap between input and is. Seems unlikely people rely on defaultValue as an attribute. As a property is not officially shipped yet so not breaking.
autoFocus is interesting because I noticed that we only allow it on specific elements, and not for example something with a tabIndex on it which seems like an existing React bug. So actually setting autoFocus can at least give you a chance to handle that in the custom element.
multiple and muted will be reinstated to optionally use properties with the enableCustomElementPropertySupport flag on.
| } | ||
| break; | ||
| } | ||
| case 'rowSpan': |
There was a problem hiding this comment.
wait why do we have special behavior for rowSpan and not colSpan
There was a problem hiding this comment.
You tell me. Turns out you added rowSpan as a numeric but not colSpan. Probably just an oversight?
Unreasonably so. It makes me feel empowered when I can follow the cost for each case. |
github-actions bot
pushed a commit
that referenced
this pull request
Apr 4, 2023
…26546) There are four places we have special cases based off the DOMProperty config: 1) DEV-only: ReactDOMUnknownPropertyHook warns for passing booleans to non-boolean attributes. We just need a simple list of all properties that are affected by that. We could probably move this in under setProp instead and have it covered by that list. 2) DEV-only: Hydration. This just needs to read the value from an attribute and compare it to what we'd expect to see if it was rendered on the client. This could use some simplification/unification of the code but I decided to just keep it simple and duplicated since code size isn't an issue. 3) DOMServerFormatConfig pushAttribute: This just maps the special case to how to emit it as a HTML attribute. 4) ReactDOMComponent setProp: This just maps the special case to how to emit it as setAttribute or removeAttribute. Basically we just have to remember to keep pushAttribute and setProp aligned. There's only one long switch in prod per environment. This just turns it all to a giant simple switch statement with string cases. This is in theory the most optimizable since syntactically all the information for a hash table is there. However, unfortunately we know that most VMs don't optimize this very well and instead just turn them into a bunch of ifs. JSC is best. We can minimize the cost by just moving common attribute to the beginning of the list. If we shipped this, maybe VMs will get it together to start optimizing this case but there's a chicken and egg problem here and the game theory reality is that we probably don't want to regress. Therefore, I intend to do a follow up after landing this which reintroduces an object indirection for simple property aliases. That should be enough to make the remaining cases palatable. I'll also extract the most common attributes to the beginning or separate ifs. Ran attribute-behavior fixture and the table is the same. DiffTrain build for [eeabb73](eeabb73)
sebmarkbage
added a commit
that referenced
this pull request
Apr 4, 2023
This is a follow up to #26546 This is strictly a perf optimization since we know that switches over strings aren't optimally implemented in current engines. Basically they're a sequence of ifs. As a result, we're better off putting the unusual cases in a Map and the very common cases in the beginning of the switch. We might be better off putting very common cases in explicit ifs - just in case the engine does optimize switches to a hash table which is potentially worse. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 13, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
sophiebits
added a commit
to sophiebits/react
that referenced
this pull request
Apr 14, 2023
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.
sophiebits
added a commit
to sophiebits/react
that referenced
this pull request
Apr 14, 2023
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.
kassens
added a commit
to kassens/react
that referenced
this pull request
Apr 17, 2023
…ramming (facebook#26546)" This reverts commit eeabb73.
sophiebits
added a commit
to sophiebits/react
that referenced
this pull request
Apr 18, 2023
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.
sophiebits
added a commit
that referenced
this pull request
Apr 18, 2023
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to #26546.
kassens
pushed a commit
that referenced
this pull request
Apr 21, 2023
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to #26546.
jeongshin
pushed a commit
to jeongshin/react-native
that referenced
this pull request
May 7, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…acebook#26546) There are four places we have special cases based off the DOMProperty config: 1) DEV-only: ReactDOMUnknownPropertyHook warns for passing booleans to non-boolean attributes. We just need a simple list of all properties that are affected by that. We could probably move this in under setProp instead and have it covered by that list. 2) DEV-only: Hydration. This just needs to read the value from an attribute and compare it to what we'd expect to see if it was rendered on the client. This could use some simplification/unification of the code but I decided to just keep it simple and duplicated since code size isn't an issue. 3) DOMServerFormatConfig pushAttribute: This just maps the special case to how to emit it as a HTML attribute. 4) ReactDOMComponent setProp: This just maps the special case to how to emit it as setAttribute or removeAttribute. Basically we just have to remember to keep pushAttribute and setProp aligned. There's only one long switch in prod per environment. This just turns it all to a giant simple switch statement with string cases. This is in theory the most optimizable since syntactically all the information for a hash table is there. However, unfortunately we know that most VMs don't optimize this very well and instead just turn them into a bunch of ifs. JSC is best. We can minimize the cost by just moving common attribute to the beginning of the list. If we shipped this, maybe VMs will get it together to start optimizing this case but there's a chicken and egg problem here and the game theory reality is that we probably don't want to regress. Therefore, I intend to do a follow up after landing this which reintroduces an object indirection for simple property aliases. That should be enough to make the remaining cases palatable. I'll also extract the most common attributes to the beginning or separate ifs. Ran attribute-behavior fixture and the table is the same.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…k#26551) This is a follow up to facebook#26546 This is strictly a perf optimization since we know that switches over strings aren't optimally implemented in current engines. Basically they're a sequence of ifs. As a result, we're better off putting the unusual cases in a Map and the very common cases in the beginning of the switch. We might be better off putting very common cases in explicit ifs - just in case the engine does optimize switches to a hash table which is potentially worse. --------- Co-authored-by: Sophie Alpert <git@sophiebits.com>
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Since this is an observable behavior and is hard to think about, seems good to have tests for this. The expected value included in each test is the behavior that existed prior to facebook#26546.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
…26546) There are four places we have special cases based off the DOMProperty config: 1) DEV-only: ReactDOMUnknownPropertyHook warns for passing booleans to non-boolean attributes. We just need a simple list of all properties that are affected by that. We could probably move this in under setProp instead and have it covered by that list. 2) DEV-only: Hydration. This just needs to read the value from an attribute and compare it to what we'd expect to see if it was rendered on the client. This could use some simplification/unification of the code but I decided to just keep it simple and duplicated since code size isn't an issue. 3) DOMServerFormatConfig pushAttribute: This just maps the special case to how to emit it as a HTML attribute. 4) ReactDOMComponent setProp: This just maps the special case to how to emit it as setAttribute or removeAttribute. Basically we just have to remember to keep pushAttribute and setProp aligned. There's only one long switch in prod per environment. This just turns it all to a giant simple switch statement with string cases. This is in theory the most optimizable since syntactically all the information for a hash table is there. However, unfortunately we know that most VMs don't optimize this very well and instead just turn them into a bunch of ifs. JSC is best. We can minimize the cost by just moving common attribute to the beginning of the list. If we shipped this, maybe VMs will get it together to start optimizing this case but there's a chicken and egg problem here and the game theory reality is that we probably don't want to regress. Therefore, I intend to do a follow up after landing this which reintroduces an object indirection for simple property aliases. That should be enough to make the remaining cases palatable. I'll also extract the most common attributes to the beginning or separate ifs. Ran attribute-behavior fixture and the table is the same. DiffTrain build for commit eeabb73.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are four places we have special cases based off the DOMProperty config:
Basically we just have to remember to keep pushAttribute and setProp aligned. There's only one long switch in prod per environment.
This just turns it all to a giant simple switch statement with string cases. This is in theory the most optimizable since syntactically all the information for a hash table is there. However, unfortunately we know that most VMs don't optimize this very well and instead just turn them into a bunch of ifs. JSC is best. We can minimize the cost by just moving common attribute to the beginning of the list.
If we shipped this, maybe VMs will get it together to start optimizing this case but there's a chicken and egg problem here and the game theory reality is that we probably don't want to regress. Therefore, I intend to do a follow up after landing this which reintroduces an object indirection for simple property aliases. That should be enough to make the remaining cases palatable. I'll also extract the most common attributes to the beginning or separate ifs.
Ran attribute-behavior fixture and the table is the same.