performance issue related to lodash/isEqual function

[igorbrasileiro]

Prerequisites

• [X] I have searched the existing issues
• [X] I understand that providing a SSCCE example is tremendously useful to the maintainers.
• [X] I have read the documentation
• [ ] Ideally, I'm providing a sample JSFiddle, Codesandbox.io or preferably a shared playground link demonstrating the issue.

What theme are you using?

core

Version

5.x

Current Behavior

No response

Expected Behavior

No response

Steps To Reproduce

1. Run the gist https://gist.github.com/igorbrasileiro/754a1a0fe7bb4f8a080ca2c0ed3426e0

the Gist was made with deno bench, but can be done with other js runtime.

Environment

- OS: macOS Version 14.6
- deno: 1.46.2

Anything else?

The results of the benchmark, in all three cases, the fast-deep-equal is better than lodash/isEqual

[igorbrasileiro]

I already have changed all isEqual occurrences in the RJSF https://github.com/igorbrasileiro/react-jsonschema-form/commit/274a0a5f419517f2e151e7335b9fd0d1af046e58. Can I apply this improvement?

[igorbrasileiro]

I did a test running the playground locally with a bigger schema and big formdata and had 20% improvement. Others tests could be done

Steps to test:

1. open the playground
2. clean schema, formdata and uischema
3. click on live validation
4. fill the Schema and formData
5. wait form renders
6. click to start the browser profiling
7. click on input name (root_name)
8. press one key

version with fast-deep-equal: https://github.com/igorbrasileiro/react-jsonschema-form. This problem starts to be more serious when you have realtime forms with schema or data changes.

Results:

• at RJSF main: 549ms

• with fast-deep-equal: 419ms .

[heath-freenome]

@igorbrasileiro We have one concern about the fast-deep-equals as it has an issue where it does not detect cycles. We looked over your MR and from what we can tell you are only using it to compare formData and schema objects... So neither of them should have memory cycles. We recommend that you simply update the deepEquals() function in the utils to use fast-deep-equal instead, and change all the other places where you are changing isEqual() to deepEquals() in the utils. You can probably skip merging the formatting changes since our linter will likely fail those.

Also, we noticed that we are using isEqual() in the validator-ajv8 library so we would recommend updating it there to use deepEquals() too.

Please submit the PR for our consideration. Thanks for the good work.

[igorbrasileiro]

Ok, I will open a PR considering everything you mentioned. Thank you for the explanation too

[igorbrasileiro]

Hi @heath-freenome, thank you for the suggestion! Unfortunately, we can't replace deepEquals with fast-deep-equal because fast-deep-equal returns false when comparing nested functions, like in the example below:

function deepEquals(a: any, b: any): boolean {
  return isEqualWith(a, b, (obj: any, other: any) => {
    if (typeof obj === "function" && typeof other === "function") {
      // Assume all functions are equivalent
      // see https://github.com/rjsf-team/react-jsonschema-form/issues/255
      return true;
    }
    return fastDeepEqual(obj, other); // fallback to default isEquals behavior
  });
}

console.log(deepEquals({ foo: { bar() {} } }, { foo: { bar() {} } })) // false

Given this limitation, I suggest we keep the current deepEquals implementation. Alternatively, we could create a new wrapper function for cases where fast-deep-equal is more appropriate. What do you think?

[igorbrasileiro]

@heath-freenome I found a faster package that works with a custom comparator, to allow compare functions

https://github.com/planttheidea/fast-equals. Can you give your considerations about it?

Same benchmark I did above (gist), but with the fast-equal as baseline

[igorbrasileiro]

@heath-freenome should reopen this issue?

https://github.com/rjsf-team/react-jsonschema-form/pull/4292#issuecomment-2352847270

[igorbrasileiro]

fast-equals released a version that handles functions in comparison.

https://github.com/planttheidea/fast-equals/pull/127

[heath-freenome]

@igorbrasileiro Did you want to try to the fix again? At minimum, first try the isEquals() replacement with deepEquals() and see if any that issue recurs. Then make the change in a second PR.

[igorbrasileiro]

@heath-freenome good approach, will do it, asap.

[igorbrasileiro]

@heath-freenome did it. I still don't knowing how to have a schema that breaks as mentioned in first PR, if you have a hint, feel free to send me.

[igorbrasileiro]

@heath-freenome I opened a PR (#4446) with the second part of your suggested approach. Now replacing the lodash.isEqualWith with fast-equals.createCustomEqual 🙏.