Subclass JsonForms from ResolvedJsonForms. - #1214
Merged
Merged
Conversation
This is an alternate solution to eclipsesource#1209 (and eclipsesource#1208) which takes the route of subclassing the JsonForms to allow renderers that are not at the root to explicitly include a JsonForms that already has the schema fully resolved and avoid the expensive penalties of hasRefs checks. Fixes: eclipsesource#1209 Fixes: eclipsesource#1208
Contributor
|
LGTM, thanks! |
Contributor
Author
|
Thanks @edgarmueller!!! |
nowells
added a commit
to nowells/jsonforms
that referenced
this pull request
Mar 12, 2020
I introduced a fix in eclipsesource#1214 to fix performance of rendering lots of fields. The problem stemming from `JsonFormsDispatchRenderer` calling `hasRefs` while `ResolvedJsonFormsDispatchRenderer` does not, as only the first JsonForm needs to handle processing resolving refs. Right now we are doing a lot of wasted `object-hash` comparisons in the `hasRefs` memoized check due to using the wrong component in our `material` and `vanilla` components.
sdirix
pushed a commit
to nowells/jsonforms
that referenced
this pull request
Mar 20, 2020
During refactorings the performance fix introduced in eclipsesource#1214 was lost. The fix is now integrated again. In general we should use the 'ResolvedJsonFormsDispatch' as often as possible as the 'hasRef' and 'resolveRef' calls are very expensive.
sdirix
pushed a commit
that referenced
this pull request
Mar 20, 2020
During refactorings the performance fix introduced in #1214 was lost. The fix is now integrated again. In general we should use the 'ResolvedJsonFormsDispatch' as often as possible as the 'hasRef' and 'resolveRef' calls are very expensive.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
When running with @jsonforms/react there is a significant performance issue when rendering a form with a lot of fields. I believe I have tracked this down to excessive calls to hasRefs. As I understand the code, that deferred call to resolve the refs on the schema, only needs to happen once on the root JsonForms component, as it will pass/provide the already resolve schema down to children, however, all of the children are also performing the hasRefs check, even though there will be no refs as the root JsonForms component has resolve them. hasRefs is an expensive call, especially when run on a large form. I was able to drop render time from 30s to 3 seconds by removing that call.
This is an alternate solution to #1209 (and #1208) which takes the route of subclassing the JsonForms to allow renderers that are not at the root to explicitly include a JsonForms that already has the schema fully resolved and avoid the expensive penalties of hasRefs checks.
Fixes: #1209
Fixes: #1208