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

TreeDropdownField is very slow when there's lots of nodes in the query #2265

Closed
maxime-rainville opened this issue Sep 17, 2018 · 8 comments
Closed

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Sep 17, 2018

This is related to #2250 (comment)

TreeDropownField is populated by doing AJAX calls to a tree action. These calls can take 15sec+ on a key project. Maybe fixing #2250 will fix this call as well ... or we might need a custom implementation for the TreeDropdownField.

@chillu
Copy link
Member

chillu commented Sep 17, 2018

For the record, I think we should change TreeDropdownField->tree() to operate on DataList rather than the evil MarkedSet. As far as I can tell, we've used this as a convenient way to retrieve a list of records, plus pre-cache the next level down so it's faster to navigate the component. This can be achieved in different ways as well.

Looking at the API surface of this class, we need to keep the terminology ("tree", "children", etc). But there is no reason to "mark" a tree, which seems to be the expensive bit. If you're in search mode, the results are a simple list, there's no "tree view" (other than breadcrumbs). If you're not in search mode, there's no "mark to expose" required to expand a tree to a certain node, since you're always operating in a flat list (with child navigation actions).

I've checked back with the refactoring work (silverstripe/silverstripe-framework#6141), and it looks like we don't have a "legacy view" of this any more - There's TreeDropdownFieldEntwine.js to wrap the React field, but there doesn't seem to be a use for rendering an actual tree as part of this field any more.

This would obviously be a bit of a larger refactoring, but a worthwhile one. We're likely to throw away the entire cursed "tree marking" logic when we re-implement the tree in React, and moving this level of state management to the client instead. This gets us closer to this goal.

@chillu
Copy link
Member

chillu commented Sep 17, 2018

FYI this is a payload from a three level small tree view. It's including all three levels (since it's under the node threshold of ~250). That's neat, but not essential. We could just implement the first two levels with a simple loop (no complex tree marking), and then use further XHR requests to dig in further. https://gist.github.com/chillu/4e17e3e08d7134dcaf952239bc42d101

@maxime-rainville
Copy link
Contributor Author

maxime-rainville commented Sep 17, 2018

I had a quick look specifically at the TreeDropdown on my box. When looking at the settings for Page ID 7772 ( Home > Destinations > North Island), I get 3 calls to the tree action on TreeDropdownField:

  • 1 call to populate the EditorGroups field which comes back in ~4.7 sec
  • 1 call to populate the ViewerGroups field which comes back in ~9.4 sec
  • 1 call to populate the ParentID field which comes back in ~15.7 sec

Those 3 calls are running concurrently. If you run them individually, they come back in 4.7 sec, 4.7 sec and 6.0 sec respectively. I tried firing off that same request in 5 different tabs and some of those tree calls were taking over a minute ... which is kind of nuts.

One confusing bit is that there's only 4 groups in the site and fetching them takes 4.7 sec. So there's something else at play here other than the volume of data. I'll concentrate on this part first.

A second thing we could look at is delaying the call until the user actually interacts with the TreeDownField. I'll have a look at that part afterwards.

@maxime-rainville
Copy link
Contributor Author

Found the culprit! It's CMSMain::getArchiveWarningMessage() ... again.

Each call to the tree endpoint forces us to reinstanciate CMSPageSettingsController and call getEditForm() which calls getArchiveWarningMessage() each time.

If I comment out the getArchiveWarningMessage call my concurrent tree calls come down to 0.1 sec, 0.2 sec and 1.7 sec.

@maxime-rainville maxime-rainville self-assigned this Sep 18, 2018
@maxime-rainville
Copy link
Contributor Author

Once we implement a fix for silverstripe/silverstripe-admin#621, the getArchiveWarningMessage() overhead will go away. I've informed the key project team and they've got a way to bypass the getArchiveWarningMessage() issue.

Delaying the tree call could be beneficial, but much less of a plus in this context.

@blueo
Copy link

blueo commented Sep 20, 2018

We've found some project code that was the major contributor to this bug so this can probably be closed. One thing to note, however, is that we originally tried setting the node_threshold_total config option. This sped up the load but broke the search. Not sure if you want to address that as part of this issue or if it should be logged separately.

@chillu
Copy link
Member

chillu commented Sep 21, 2018

Longer term solution: Take the XHR out of nested controllers, and make it talk GraphQL. #2271

@maxime-rainville
Copy link
Contributor Author

Our key project has found alternative solution to this, that is very specific to them. There's nothing else left to look at with this card, so I'll just close it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants