5.3.0: Array items perform much worse

MatinF commented 1 year ago

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
[X] 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.0.0-beta.18 vs. 5.0.0-beta.19

Current Behavior

We wanted to test out the latest update to rjsf (upgrading from 5.0.0 beta11 to 5.3.0) in order to benefit from the new ability to 'copy' array items, which was added as a feature in the latest release.

Unfortunately, the performance of the updated version is much worse when it comes to adding/removing array items - to an extent where it now takes ~1-2 seconds to add a new entry, up from <0.1 seconds in the previous release. After isolating the release introducing this, it seems to start after 5.0.0-beta.18.

I am unable to replicate this in the live playground, though I am unsure why.

Expected Behavior

I would expect the performance to be similar to previous versions.

Steps To Reproduce

1) Use a standard rjsf setup with 5.0.0-beta.18 vs. 5.0.0-beta.19

2) Use below custom array template:

3) To replicate this, test with the below formdata (config) and schema/uischema. The slow performance can be seen by going to CAN CH1 and adding entries in the Transmit list section. json-schema.zip

nickgros commented 1 year ago

@MatinF I tried to re-create this in CodeSandbox and didn't notice issues with array performance https://codesandbox.io/s/rjsf-3517-repro-679s2u

Can you create your own example or modify the above so we can get to the bottom of this (or clarify further how to see the issue if it is present in that example)?

MatinF commented 1 year ago

I've been trying to replicate your dependency setup in my own application, but struggling to achieve this due to some legacy dependencies. I recognize that there is not a performance issue in your sandbox, so I am not sure why there is on my side.

The app I'm testing this with is below: https://github.com/CSS-Electronics/config-editor-base

You can install it by running npm install in the root and in the example/ folder, followed by running npm start in both folders. If you then load attached configuration file you'll be able to load up the editor: config-01.07.zip

I'm guessing the issue is related to some of the outdated dependencies I have, such as React 16.0.0 and other things. I tried various steps such as updating from validator v6 to v8 as in your sandbox, but the slow performance persists.

As you can see if you decide to install the app, the performance is snappy with the default version of rjsf in the repository - but if you update to 5.3.0 it becomes much slower.

heath-freenome commented 1 year ago

@MatinF Wow that a lot of code you've written. I don't really have time to analyze and inspect it all to find the issue for you. I'm wondering if you are experiencing an interaction in your own code with react rerendering? Have you been able to debug into the RJSF code to see if it is there? Also, I just fixed an issue in 5.5.1 that prevented infinite recursions when using a schema with recursive refs. Can you check to see if that helped.

Cauen commented 1 year ago

@heath-freenome @nickgros Only by updating core to v5+, starts a performance problem on your example...

MatinF commented 1 year ago

I tested this with my own repository, config-editor-base, installing v5.5.2 of all the rjsf modules - but the performance issue remains unaffected. I think the issue is linked to some change made after 5.0.0-beta.18.

heath-freenome commented 1 year ago

I tested this with my own repository, config-editor-base, installing v5.5.2 of all the rjsf modules - but the performance issue remains unaffected. I think the issue is linked to some change made after 5.0.0-beta.18.

From what you shared the problem started in 5.0.0-beta.19 right?

Cauen commented 1 year ago

@heath-freenome 5.0.0-beta.1 in my tests. You can check by only changing version in codesandbox

heath-freenome commented 1 year ago

@Cauen First off, I haven't had a chance to dig deep into your example yet. Basically you are saying that v5 has performance issues that v4 doesn't have. A lot changed between 4 and 5. Are you willing to slowly reduce the size/complexity of the schema in example until the performance issue seems to go away and then put back the bit that causes it? Or, better yet, are you able to debug the current codebase enough to find out where the issue is happening? I basically only get a few hours on the weekend to tackle stuff and this seems like a large thing to tackle. Thanks

MatinF commented 1 year ago

@heath-freenome Fully understand that this is a rather cumbersome thing to test. I did specifically go through each revision on my end and can confirm for certain that it starts after 5.0.0-beta.18 - i.e. with 5.0.0-beta.19 being the first time I experience the problem. It is very clear cut. Every subsequent release has the same issue. I did not experience it in versions prior to 5.0.0-beta.19, though.

heath-freenome commented 1 year ago

@MatinF Are you using liveOmit and omitExtraData? We just added support for the OpenAPI discriminator for oneOf and anyOf. This could speed up your example a bunch. We are only supporting the propertyName of discriminator, and not the mappings. Using your schema, here's an updated fragment:

dependencies: {
        id_format: {
          discriminator: {
            propertyName: "id_format"
          },
          oneOf: [
            {
              properties: {
                id_format: {
                  enum: [0]
                },
                id: {
                  pattern: "^([0-7][a-fA-F0-9]{2}|[a-fA-F0-9]{1,2})$",
                  maxLength: 3
                },
                id_mask: {
                  pattern: "^([0-7][a-fA-F0-9]{2}|[a-fA-F0-9]{1,2})$",
                  maxLength: 3
                }
              }
            },
            {
              properties: {
                id_format: {
                  enum: [1]
                },
                id: {
                  pattern: "^([0-1][a-fA-F0-9]{7}|[a-fA-F0-9]{1,7})$",
                  maxLength: 8
                },
                id_mask: {
                  pattern: "^([0-1][a-fA-F0-9]{7}|[a-fA-F0-9]{1,7})$",
                  maxLength: 8
                }
              }
            }
          ]
        }
      },
      additionalProperties: false,
      required: ["chn", "id_format", "id", "id_mask"]
    },

MatinF commented 1 year ago

Hi again,

Yes, we use both of those, see below:

<FormWithNav
               validator={validator}
                omitExtraData={true}
                liveOmit={true}
                liveValidate={true}
                noHtml5Validate={true}
                schema={schemaContent ? schemaContent : {}}
                uiSchema={uiContent ? uiContent : {}}
                formData={configContent ? configContent : {}}
                onSubmit={this.onSubmit}
                onChange={this.handleChange}
                onError={this.handleError}
                onNavChange={this.onNavChange.bind(this)}
                templates={{ArrayFieldTemplate}}
                activeNav={activatedTab}
              >

I'm not sure how I would go about testing the impact of the OpenAPI discriminator you mention?

As mentioned before, we have never had any performance issues at all until the specific update 5.0.0-beta.19, i.e. I don't think this is related to an under-performing implementation in the Rule Schema, but rather I think this is due to some change implemented within that update that somehow affects our Rule Schema particularly hard.

heath-freenome commented 1 year ago

@MatinF So the release you have pointed to fixed a bug with toPathSchema() so that it handled oneOf/anyOf schemas (before that it didn't). toPathSchema() is used to computed what fields need to be checked in the liveOmit/omitExtraData. So it make sense that you experienced a slow-down when this fix was added. There definitely were issues with recursive references significantly slowing down the toPathSchema() function but those were fixed. The reason I suggested using the Open API descriptor is that it would speed up the detection of the schema that matches formData when there is a oneOf that contains objects that rely on the selection being based on that value of a single field of that object.

If you were to check out the speed of your code before and after adding them, I would hope you will see a speed up of the UI

MatinF commented 1 year ago

Ok just to check: If I am to test the effect of this, would I be able to do so by simply disabling the liveOmit / omitExtraData and seeing if that suffices to fix the performance issues?

If that helps, how would I enable the use of the Open API descriptor? I'm not familiar with this, so I am unsure if this is a built in feature of rjsf, or a separate project?

heath-freenome commented 1 year ago

Ok just to check: If I am to test the effect of this, would I be able to do so by simply disabling the liveOmit / omitExtraData and seeing if that suffices to fix the performance issues?

If that helps, how would I enable the use of the Open API descriptor? I'm not familiar with this, so I am unsure if this is a built in feature of rjsf, or a separate project?

Yes, turning off both of those flags should gain you performance.

You need only add the open API descriptor to your schema and RJSF will detect and use it to speed up the selection of the oneOf based upon the single field. I can get on a discord call with you sometime to walk you through it if that helps.

MatinF commented 1 year ago

Hi again,

Just as a bit of an update on this topic: We have been further investigating potential ways to resolve the issue with array items for our Rule Schema. However, in the more recent versions of the playground, it appears that the overall performance with our Rule Schema has completely tanked. As such, I think something more fundamental is going on in terms of how our type of Rule Schema is handled in the more recent releases.

You can verify this via the playground link below.

As you should be able to verify, simply entering a value in the first field (or any field) comes with a massive "lag" of multiple seconds. This is not at all the case in the version we're currently running our own app with (i.e. the versions up until and excluding 5.0.0-beta.19).

I'm not sure it's the same thing causing this recently observed playground lag and the more array-related issue pointed out in this issue. However, I'm guessing there may be some related underlying factor - e.g. because our Schema may be considered "large" (87 kb) or heavily nested. Any inputs on this would be of great interest as we would like to be able to update to the latest versions, but the performance blocks this - now also evident from the playground itself

xyy7260 commented 1 year ago

@MatinF Did you solve it?

MatinF commented 1 year ago

No in contrast, the latest rjsf playground exhibits performance issues for all fields when we load our Schema. This is not the case in the older versions as discussed in this thread and in the latest rjsf version it seems the issue of performance is no longer limited to array entries.

You should be able to replicate this performance issue by using the playground link I added in my previous post.

xyy7260 commented 1 year ago

@MatinF Ok, I wonder if the next release will fix the performance issues

heath-freenome commented 1 year ago

@MatinF I'm using your playground link and when I turn off omit extra data and live omit the response time goes from a second or 2 delay to about 1/4 of a second. Maybe it make sense to add some performance tuning optional parameters to the Form. If you want live omit, maybe we can add a skipOneOf or skipArrays flags to return things to where they were before the bug fix. Also, maybe there is a more performant way of omitting live data that doesn't rely on the pathSchema? All of these things require time and a lot of debugging, which sadly, I do not have much of in the 2 hours a week I am able to spend on the library.

MatinF commented 1 year ago

Thanks for providing inputs - fully understand that there is limited time for this. For now, we're using the older version which works OK for us right now, so this is more a focus for longer-term as we'd of course want to be able to keep up with new features/improvements eventually.

I tried testing with the live omit and omit extra data turned off and it does improve it a bit - but if you e.g. write some text in the top, it still seems very laggy on my end compared to the older versions. I'm not sure what is causing that, however.

rjsf-team / react-jsonschema-form