Optimize condition and field array performance #1017

Hyperkid123 · 2021-03-26T14:40:06Z

Description
The issue was that any field wrapped in the condition is subject to render, even if the when value did not change. This introduced improved FormSpy that listens only on a set of values in the form state. The effect of the new spy is that we no longer trigger render cycle in every condition wrapped field and by that extension skips the condition evaluation because it's not required.

See profiler snapshots and examples in the linked issue.

codecov · 2021-03-26T14:42:16Z

Codecov Report

Merging #1017 (c0a455c) into master (44473bc) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
+ Coverage   84.80%   85.06%   +0.26%     
==========================================
  Files         199      200       +1     
  Lines        3264     3321      +57     
  Branches     1115     1130      +15     
==========================================
+ Hits         2768     2825      +57     
  Misses        496      496

Impacted Files	Coverage Δ
...nt-component-mapper/src/field-array/field-array.js	`98.33% <100.00%> (+0.02%)`	⬆️
...nt-component-mapper/src/field-array/field-array.js	`100.00% <100.00%> (ø)`
...on-component-mapper/src/field-array/field-array.js	`100.00% <100.00%> (ø)`
...ui-component-mapper/src/field-array/field-array.js	`100.00% <100.00%> (ø)`
...f4-component-mapper/src/field-array/field-array.js	`100.00% <100.00%> (ø)`
...t-form-renderer/src/form-renderer/form-renderer.js	`100.00% <100.00%> (ø)`
...act-form-renderer/src/form-renderer/render-form.js	`100.00% <100.00%> (ø)`
...c/get-condition-triggers/get-condition-triggers.js	`100.00% <100.00%> (ø)`
...t-form-renderer/src/use-field-api/use-field-api.js	`91.66% <100.00%> (+0.45%)`	⬆️
...ir-component-mapper/src/field-array/field-array.js	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 750d8cc...c0a455c. Read the comment docs.

Hyperkid123 · 2021-03-29T06:51:43Z

I have thought of one possible issue and that is we are not accepting the dot notation when constructing the reduced values object. Ill adjust the code.

rvsia · 2021-03-29T08:43:33Z

As we discussed in DMs, there is the issue with an unwanted registering fields that are being created in the condition wrapper. We should make this setting opt-in and describe the caveat in the documentation, or we could update the getRegisteredFields to ignore fields created by conditions (however, the question is then how to know what fields are created by conditions or by the useFieldHook)

Or we can wait until our state manager is implemented - then it would be simple to implement FieldSpy. 😈

edit: Theoretically we can make an array in a ref (so no rerenders) and update it in the useFieldApi hooks - and then return the array in the method instead of the FF's one. 🤔 However, not sure if there could be any side-effects of this solution.

rvsia · 2021-03-31T08:42:59Z

schema for checking the wizard

const wizardSchema = {
  fields: [
    {
      component: componentTypes.WIZARD,
      name: 'wizzard',
      buttonLabels: {},
      title: 'Cokoliv',
      fields: [
        { name: 'first', nextStep: ({values: {path}}) => path ? 'path1' : 'path2', fields: [{
          name: 'path',
          component: componentTypes.SWITCH,
          label: 'Path 1?'
        }]},
        {
          name: 'path1',
          nextStep: 'summary',
          fields: [{
            component: componentTypes.TEXT_FIELD,
            name: 'text1',
            label: 'text1',
            validate: [{type: 'required'}]
          }]
        },
        {
          name: 'path2',
          nextStep: 'summary',
          fields: [{
            component: componentTypes.TEXT_FIELD,
            name: 'text2',
            label: 'text2',
            validate: [{type: 'required'}]
          }]
        },
        {
          name: 'summary',
          fields: [{
            name: 'summary',
            component: componentTypes.TEXTAREA,
            label: 'summary',
            condition: {
              or: [
                {when: 'text1', isNotEmpty: true},
                {when: 'text2', isNotEmpty: true},
              ]
            }
          }]
        }
      ]
    }
  ]
};

Hyperkid123 · 2021-03-31T08:52:33Z

@rvsia I've added a custom field registration method. The wizard works with the new registration. There I've kept the FF getRegisteredFields under an alias.

That part is ready. I will check the field array and what might be the bottleneck here.

rvsia · 2021-03-31T08:55:03Z

packages/react-form-renderer/src/form-renderer/form-renderer.js

@@ -26,9 +26,20 @@ const FormRenderer = ({
  ...props
 }) => {
  const [fileInputs, setFileInputs] = useState([]);
+  const [registeredFields, setRegisteredFields] = useState({});


What about to use ref to not trigger any renders?

I don't think this triggers any additional renders of the children 🤔 Let me check the profiler. But, using ref might cause some race conditions because we can't use a callback to override the value.

Oh yeah, it does 🤦 we don't shield the render from parent re-renders. I'm just gonna slap the ref there and we will see what it does.

You don't have to use a callback. It's an array, so you can modify it and the reference is still the same. And you don't have to use an object: use array with duplicates and the function will return a Set from the array.

OK it almost totally undid all the work. We can mimic the callback from setState which will make using ref safer.

So register just pushes the name into the array, unregister removes the first occurrence of the name and getRegistered returns the array without duplicates.

rvsia · 2021-03-31T09:08:11Z

packages/react-form-renderer/src/form-renderer/render-form.js

+
+  const name = internalTriggers.shift();
+  return (
+    <FieldProvider skipRegistration name={name} subscription={{ value: true }} render={({input: {value}}) => (


I would still use the FF field - we don't need any of our useFieldApi features (and so we don't have to use any skipRegistration)

rvsia · 2021-03-31T09:08:33Z

packages/react-form-renderer/src/form-renderer/render-form.js

@@ -111,6 +159,8 @@ SingleField.propTypes = {
  resolveProps: PropTypes.func
 };

-const renderForm = (fields) => fields.map((field) => (Array.isArray(field) ? renderForm(field) : <SingleField key={field.name} {...field} />));
+const renderForm = (fields) => {
+  return fields.map((field) => (Array.isArray(field) ? renderForm(field) : <SingleField key={field.name} {...field} />));


return block

Hyperkid123 · 2021-03-31T12:02:42Z

@rvsia I think it's ready. Feel free to dive into the code.

rvsia · 2021-03-31T12:10:39Z

packages/ant-component-mapper/package.json

@@ -69,7 +69,8 @@
    "react-dom": ">=16.13.0"
  },
  "dependencies": {
-    "@data-driven-forms/common": "*"
+    "@data-driven-forms/common": "*",
+    "lodash": "^4.17.21"


Isn't lodash dependency of form-renderer?

Yes, but now lodash is a direct dependency of the package and should be listed.

rvsia · 2021-03-31T12:22:15Z

packages/react-form-renderer/src/form-renderer/form-renderer.js

+  const internalRegisterField = (name) => {
+    setRegisteredFields(prev => prev[name] ? ({...prev, [name]: prev[name] + 1}) : ({...prev, [name]: 1}));
+  };
+
+  const internalUnRegisterField = (name) => {
+    setRegisteredFields(({[name]: currentField, ...prev}) => currentField && currentField > 1 ? ({[name]: currentField - 1, ...prev}) : prev);
+  };
+
+  const internalGetRegisteredFields = () => Object.entries(registeredFields.current).reduce((acc, [name, value]) => value > 0 ? [...acc, name] : acc, []);
+


What about to do what I proposed (this seems unnecessary complex to me):

const registeredFields = useRef([]);

register:

const internalRegisterField = (name) => registeredFields.current.push(name)

unregister:

function removeElement(array, elem) { var index = array.indexOf(elem); if (index > -1) { array.splice(index, 1); } } const internalUnRegisterField = (name) => removeElement(registeredFields.current, name)

internalGetRegisteredFields

const internalGetRegisteredFields = () => new Set(registeredFields.current)

There is a couple of potential issues. First, I don't like the idea of having the same item multiple times inside the array. Using an object is slightly more resource-efficient than looking for indexes.

Another problem might be the usage of push which can create an out-of-sync structure when we re-create the array (splice in removeElement). Theoretically, we can push into an array that no longer exists.

Also this:

const internalGetRegisteredFields = () => new Set(registeredFields.current)

Would return an instance of a set instead of an array. We would need either Array.from or [...x] destructor to actually get an array to preserve current API. So we need some value transformation anyway.

rvsia · 2021-03-31T12:22:31Z

packages/react-form-renderer/src/form-renderer/render-form.js

+    <ConditionTriggerDetector triggers={triggers} condition={condition} field={field}>
+      {children}
+    </ConditionTriggerDetector>
+  );


strange formatting here

Hyperkid123 · 2021-03-31T13:15:40Z

🎉 This PR is included in version 3.3.2 🎉

The release is available on

react-form-renderer (@latest dist-tag)

Demo can be found here!

fix(renderer): optimize condition rendering.

3dfc22e

Hyperkid123 added renderer React form renderer PR Technical debt labels Mar 26, 2021

Hyperkid123 requested a review from rvsia March 26, 2021 14:40

chore(renderer): add tests for name condition trigger extractor

640293d

Hyperkid123 changed the title ~~fix(renderer): optimize condition rendering.~~ [WIP] fix(renderer): optimize condition rendering Mar 29, 2021

fix(renderer): use lodash like path for new condition

678dc37

rvsia reviewed Mar 31, 2021

View reviewed changes

Hyperkid123 force-pushed the renderer-optimization branch from ee0d6a9 to 044f6de Compare March 31, 2021 09:04

rvsia reviewed Mar 31, 2021

View reviewed changes

Hyperkid123 force-pushed the renderer-optimization branch from 044f6de to d5a7ed8 Compare March 31, 2021 09:32

fix(renderer): use custom registerField function

4fa3cd5

Hyperkid123 force-pushed the renderer-optimization branch from d5a7ed8 to 4fa3cd5 Compare March 31, 2021 09:56

fix(all): memoize ArrayItem in field array

50e47d6

Hyperkid123 changed the title ~~[WIP] fix(renderer): optimize condition rendering~~ Optimize condition and field array performance Mar 31, 2021

rvsia reviewed Mar 31, 2021

View reviewed changes

chore(renderer): code formatting

c0a455c

rvsia approved these changes Mar 31, 2021

View reviewed changes

Hyperkid123 merged commit 75096be into master Mar 31, 2021

Hyperkid123 deleted the renderer-optimization branch March 31, 2021 13:07

Hyperkid123 added the released label Mar 31, 2021

Optimize condition and field array performance #1017

Optimize condition and field array performance #1017

Uh oh!

Conversation

Hyperkid123 commented Mar 26, 2021

Uh oh!

codecov bot commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hyperkid123 commented Mar 29, 2021

Uh oh!

rvsia commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvsia commented Mar 31, 2021

Uh oh!

Hyperkid123 commented Mar 31, 2021 • 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!

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!

Hyperkid123 commented Mar 31, 2021

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!

Hyperkid123 commented Mar 31, 2021

Uh oh!

Uh oh!

codecov bot commented Mar 26, 2021 •

edited

Loading

rvsia commented Mar 29, 2021 •

edited

Loading

Hyperkid123 commented Mar 31, 2021 •

edited

Loading