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

feat: improve deepEquals performance #4292

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

igorbrasileiro
Copy link
Contributor

@igorbrasileiro igorbrasileiro commented Sep 9, 2024

Reasons for making this change

This PR improves the RJSF performance when comparing objects deeply.
fixes #4291

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@igorbrasileiro igorbrasileiro changed the title feat: improve deepEquals comparators feat: improve deepEquals performance Sep 9, 2024
Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some documentation and then I'll approve

packages/utils/src/deepEquals.ts Show resolved Hide resolved
packages/utils/src/deepEquals.ts Show resolved Hide resolved
@igorbrasileiro
Copy link
Contributor Author

Added the JSDoc for new functions.

@heath-freenome
Copy link
Member

Can you also fix the merge conflict in the CHANGELOG.md? Otherwise looks good! How is the performance of fast-equals vs fast-deep-equals?

@igorbrasileiro
Copy link
Contributor Author

I pointed in the issue the performance of the customDeepEqual. But the results
image

The deepEqualsFunction

const customDeepEqual = createCustomEqual({
  createInternalComparator: (
    comparator: (a: any, b: any, state: State<any>) => boolean,
  ) => {
    return (
      a: any,
      b: any,
      _idxA: any,
      _idxB: any,
      _parentA: any,
      _parentB: any,
      state: State<any>,
    ) => {
      if (typeof a === "function" && typeof b === "function") {
        // Assume all functions are equivalent
        // see https://github.com/rjsf-team/react-jsonschema-form/issues/255
        return true;
      }

      return comparator(a, b, state);
    };
  },
});

CHANGELOG.md Outdated Show resolved Hide resolved
@heath-freenome heath-freenome merged commit 514ea85 into rjsf-team:main Sep 9, 2024
5 checks passed
@benjdlambert
Copy link
Contributor

We seem to be getting test failures for this upgrade with maximum call stack exceeded backstage/backstage#26639 don't have time to put together a reproduction repo just now, but will try and get to it this week. Just a heads up for now.

@igorbrasileiro
Copy link
Contributor Author

@benjdlambert thank you for let me know about it. I will try to reproduce this error and fix it.

Does your props have circularity?

@igorbrasileiro
Copy link
Contributor Author

igorbrasileiro commented Sep 12, 2024

I discovered how to reproduce. I reverted the deepEquals fn and the problem happened, so the issue isn't the way the fast-equals compare, it was the refactor.

Edit: I was testing on a schema with issues in one of the anyOfs, I added a null option to the anyOf and the loop stopped (explanation here #4262).

@heath-freenome
Copy link
Member

@igorbrasileiro Is it possible to programmatically fix it such that the null option is unnecessary?

@igorbrasileiro
Copy link
Contributor Author

@heath-freenome I don't have enough context about the JSON schema spec and where to start investigating this issue, but I could implement a fix. I think my issue is different from that of @benjdlambert .

Regarding the issue related to this PR, I haven't found a way to reproduce it. Do you know or have a hint?

@heath-freenome
Copy link
Member

heath-freenome commented Sep 12, 2024

@igorbrasileiro I know that @nickgros is working on an issue related to this where certain schemas cause an infinite recursion. At the same time, I wonder if your deep comparison changes are affecting a situation where isEqual() returns a value that differs from deepEquals() thereby causing infinite recursion where it was caught before. For your situation at least, if you could debug the before/after results to see how isEqual() vs deepEquals() causes the algorithms around infinite recursion to change, and then either fixing it (if you clearly introduced a bug) or reaching back out if it doesn't make sense to you...

@igorbrasileiro
Copy link
Contributor Author

igorbrasileiro commented Sep 12, 2024

Sure, that sounds good to me. What do you think about reverting this PR, then I try to find the issue and reimplement it with the fix?

Edit: Would be good to know how to reproduce the issue.

@heath-freenome
Copy link
Member

Sure, that sounds good to me. What do you think about reverting this PR, then I try to find the issue and reimplement it with the fix?

Edit: Would be good to know how to reproduce the issue.

@igorbrasileiro That makes sense... Can you push the PR and then also, work on fixing the problem you experienced. I agree understanding the problem would be super helpful. It's clear that you already have one use case to start with.

@benjdlambert Can you provide us with your schema as well to help us debug.

@nickgros If you get a chance to figure out the infinite recursion issue you are working on, feel free to chime in.

igorbrasileiro added a commit to igorbrasileiro/react-jsonschema-form that referenced this pull request Sep 13, 2024
heath-freenome pushed a commit that referenced this pull request Sep 13, 2024
* Revert "feat: improve deepEquals performance (#4292)"

This reverts commit 514ea85.

* package.json
@benjdlambert
Copy link
Contributor

benjdlambert commented Sep 16, 2024

The schemas in the tests were pretty trivial to be honest, I tried to replicate this in a small codesandbox, and think it might have been to do with the fact that the formData might have had some cyclical references in there.

There's an example here: https://codesandbox.io/p/sandbox/n6txcy so if you try and edit the form data it would break.

Granted this example is not really what you would actually do, but it was something that i could replicate the example with. I was trying to find a better way to reproduce this, as it looks like on first render it might be fine, but when it's re-rendered for some reason it causes issues with the lookup, which kind of aligns with what we were seeing in our test suite that all the things looked pretty sane, but when updating the form it hit the maximum call stack exceeded.

Sorry I can't be of much more help right now :( If I can get some more free time I can try and get a better repro example

@igorbrasileiro
Copy link
Contributor Author

igorbrasileiro commented Sep 16, 2024

@benjdlambert thank you for the codesandbox!

Changing the onChange function from your example to follow the docs, the error stops.

        onChange={(e) => {
-         setFormState(e);
+         setFormState(e.formData);
        }}

@heath-freenome can you give your considerations here? Adding the whole object received from onChange fn causes cyclic references, as far as I know this is not expected.

@igorbrasileiro
Copy link
Contributor Author

@heath-freenome as it seems to be a wrong usage of the form, can I revert the revert #4300 ? I'm considering that is not allowed pass props with cyclic reference. If is allowed I can setup the customcomparator from fast-equals to check if cyclic reference.

@benjdlambert
Copy link
Contributor

benjdlambert commented Sep 18, 2024

@igorbrasileiro I'm not convinced that crashing because of circular references in any of the props is great to be honest. I wonder if there's a way to drop the circular references ahead of the diffing or something.

I mentioned in my reproduction Granted this example is not really what you would actually do. We use just e.formData but this was still happening to us in our test cases, was just a quick way I could put together the error.

Let me see if I can get some time today to dig into what is actually happening in the broken version, as when I was debugging our tests the props that we were using looked pretty simple and we're not using cyclical references to what I could see.

igorbrasileiro added a commit to igorbrasileiro/react-jsonschema-form that referenced this pull request Sep 20, 2024
@igorbrasileiro
Copy link
Contributor Author

igorbrasileiro commented Sep 20, 2024

We use just e.formData but this was still happening to us in our test cases, was just a quick way I could put together the error.

@benjdlambert can you share the schema and formData from your test? I only get maximum call stack size exceeded with a schema with loop, but it also crashed using the form in the previous version.

Environment to reproduce, using deno, https://github.com/igorbrasileiro/rjsf-form-issue

formData: {}, schema:

{
  "definitions": {
    "first": {
      "type": "object",
      "properties": {
        "name": { "type": "string" },
        "children": { "$ref": "#/definitions/second" }
      }
    },
    "second": {
      "type": "object",
      "properties": {
        "name2": { "type": "string" },
        "children": { "$ref": "#/definitions/first" }
      }
    }
  },
  "$ref": "#/definitions/first"
}

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

Successfully merging this pull request may close these issues.

performance issue related to lodash/isEqual function
3 participants