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

Optimization problems and possible solutions #342

Closed
bbernovici opened this issue Dec 12, 2020 · 0 comments · Fixed by #1054
Closed

Optimization problems and possible solutions #342

bbernovici opened this issue Dec 12, 2020 · 0 comments · Fixed by #1054

Comments

@bbernovici
Copy link

bbernovici commented Dec 12, 2020

Describe the bug
First of all, I want to say how great this library is and how much time it saved. While working with it, I have encountered some scenarios in which perfomance became an issue for us. I will split them in 4 parts.

  • dropdownMatchSelectWidth set to false in the Select component in Multiselect.jsx and Select.jsx under antd core will disable virtual scroll, which in our case was necessary to have it enabled, otherwise Select fields that work with Big Data-amount of options would be rendered very slow.

  • Using onPropsChanged to populate the option values for Select will quickly bring the UI to a halt if you have many option tags. This is why, I think it is better to render the options only once when the component is loaded. At least, that's what I have done in my case to achieve better perfomance.

  • The property onDropdownVisibleChange={this.handleDropdownOpen} should be used to set when the the dropdown is active or not in Select because otherwise, for every change in the graph you will rerender the options again and again. I have conditionally set them to render in this way {this.state.buildOptions && this.options} where buildOptions is a boolean value holding the state for the visiblity of dropdown.

  • I think the following is the most important optimisation. I've done some profiling on it and I've attached some screenshots below to see what I'm talking about. It seems like for every rule, the library is calling getOperatorConfig which does deep merging using lodash, mainly it branches out to getFieldConfig and getWidgetForFieldOp (which also uses getFieldConfig inside) . Now, the problem that we have in this function called getFieldConfig is that it does some "copy-the-world" stuff when it shouldn't be the case, or at least I don't see the reason for why it has to do that. Getting back to my point, inside that function we are deep copying filterSettings when we are merging typeConfig and fieldConfig. The problem quickly appears when you have, let's say 1000 options, in those filterSettings and you start deep copying those for every rule that is using that field type. Filter settings never change, once the tree is rendered those settings are stateless. I see no reason to make this immutable, to pass it as a value. A fix that I thought of is described below. In summary, I have omitted the fieldSettings key which is not deemed necessary and after the merging is done I am just referencing it.

Modified snippet taken out of getFieldConfig in configUtils.js:

const typeConfig = config.types[fieldConfig.type] || {};
const fieldConfigOmitted = omitBy(fieldConfig, (value, key) => key.includes("fieldSettings"))
let ret = mergeWith({}, typeConfig, fieldConfigOmitted || {}, (objValue, srcValue, _key, _object, _source, _stack) => {
    if (Array.isArray(objValue)) {
      return srcValue;
    }
  });
ret.fieldSettings = fieldConfig.fieldSettings;

To Reproduce
Place many options in your fieldSettings for multiselect_equals and select_equals. Now that you have them in there try to use that field multiple times in the graph and see how it behaves in the long run. The perfomance will be bad, it will bring the UI almost to a halt.

Expected behavior
Faster performance than it actually offers right now.

Screenshots
Here we see how much it takes for getOperatorConfig to finish using the fixes from above:
Screenshot from 2020-12-12 10-52-30

In this one we profile the graph as it is in the current state, without the fix from the last point and we can see that it runs 14x slower:
Screenshot from 2020-12-12 10-54-11

Additional context
Haven't tested thoroughly but I haven't encountered any bugs from these changes so far. I can be wrong though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants