-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Tree Sort on first page load is incorrect #307
Comments
Hmm it looks ok to me but I just go on the live demo, I don't really have time to look into this but the code that runs the initial sort is in the Sort Service on this function slickgrid-universal/packages/common/src/services/sort.service.ts Lines 322 to 347 in a642482
There's a lot of recursion when dealing with Tree Data |
@yangjuncode some good news... I finally got back to the need to use and create a Tree Data grid (we actually have 3 different tree data grids to do) and found your reported issue on Example 5 and even other issues as well. I expect to have fixes sometime next week. The list of issues I have identified so far are:
I believe that I got proof of concept for all 3 but I need to do more testing to make sure it's all good before releasing anything and again I expect sometime next week to be done with all this. Thanks for the feedback and patience 😉 |
thanks for your hard work, looking forward to the new tree data grid. |
1. initial sort not always working 2. tree level property should not be required while providing a parentId relation 3. tree data was loading and rendering the grid more than once (at least 1x time before the sort and another time after the tree was built) while it should only be rendered once
@yangjuncode |
fix(tree): few issues found with Tree Data, fixes #307
thanks for the fix, i am just return to work. |
@ghiscoding |
hi sorry but I can't really do much about performance with that many data, Tree Data requires a lot of recursive calls and it often have to switch and convert between flat structure to hierarchical (tree) structure (and vice versa) and for that it uses recursion (for example the sort must be done on the hierarchical structure and once it's done it must convert it back to flat structure then show it on the screen because that is what SlickGrid uses, so there's a lot of conversion back and forth). SlickGrid internals (core lib) doesn't actually work with a hierarchical structure, what I've really done is to add all these convert functions to switch between flat to hierarchical and vice versa. I feel that 100k is way too much data to work with a Tree Data structure (some other grid libs might do a better job at that if their code uses hierarchical directly but SlickGrid does not, so it will always be slower). 100k data should be used with a Backend Service (OData, GraphQL) however it work with Tree Data (because it needs the entire dataset to work since it convert flat to hierarchical back and forth). The other problem with 100k rows is that all these conversions are done on your computer by the CPU and for those users who have a slower CPU, it will be worst for them. Our data usage will never go over 5k rows, so it's not too bad for our use case. Row Selection does work in every grid type and yes it does work with Tree Data as well, you can see below on the grid that I'm currently working on. You should read the Row Selection - Wiki, every information you need to know is in that Wiki. Also note that I didn't release a new version yet, I will push a new version probably next week |
since slickgrid-universal is operating fluently in 1m rows, we thought it may also ok with 100k in tree. |
no I think you misunderstand a few things, the problem is not SlickGrid itself, here's a few things to note
What is really slow is that currently the steps are
if you sort, then we need to repeat steps 1 to 4 and so on.. and so on We do all this because SlickGrid does not support Tree Data directly, it only supports a flat data structure and that is why Example 2 is fast with 50k rows but Example 5 is much slower with 50k rows. There might be some small improvements I can do (anytime we load a tree data grid it will do step 1 to 4, maybe we can omit step 2 and 3 if it's already sorted?) Some of the process are slow, I'm not sure if my methods are the fastest, I just wanted it to work (performance improvement can come later). |
@ghiscoding thanks for the details.
the problem maybe is too many new object generated for tree grid. |
the deep copy that is there cannot be removed because it's converting by reference and that mutate (change) the in input array. The only alternative I can think of would be to find an external lib that those a better job at it and is more tested. I believe your point number 2 is wrong, the only way that I know of to sort a flat structure is really to first convert tree structure then sort and finally back to flat. This 3 steps process is obviously quite expensive (CPU), if you find a better way, feel free to contribute. |
Good news, I found some libraries that provide, this one in particular un-flatten-tree which is extremely fast compare to my original implementation, now we are back to some normal numbers Doing some metrics, with 20k rows and printing the stats only for the number 3 that was my slow code (convert tree to flat) and now it's extremely faster and will work with 100k rows (I would never have that many but apparently you do). The test I'm doing is just to sort the "Title" column. with old code
with new code
...so does it work with 100k rows now? yes it does but it takes couple of seconds (instead of couple of minutes with old code)
here's a full detailed stats when loading the page with 100k rows and doing a sort on "Title" (the tree structure is only executed once and it's fast).
That was mainly POC (proof of concept) and I still have to cleanup all the code and add all unit tests but at least now it's back to a speed that is more expected of 🚀 EDIT Some more stats after I cleanup the code and fixed couple of other small issues found. Below is another test with 100k rows and a sort on the "Title" column, it's even faster than before (maybe because I cleaned up a lot of old code). However, notice that the most expensive task is the SlickGrid sort in the UI (the slowest part is not the tree sort but this call
So 5.8 sec to wait for the sort on 100k items is not bad but it still is something to wait for. With that in mind, I will add extra events so that we can show a spinner if we want while waiting for the sort to finish (maybe 2 event named |
feat(tree): improve Tree Data speed considerably, fixes #307
@yangjuncode I'm finally done with the 2nd and last PR for Tree Data, I believed that I fixed everything, the performance is improved by a lot (40-50x times like I said in previous post) and it works nicely with 100k rows (I added a button to show 25k rows in the Example 5). I also added a bunch of new events if you're dealing with large dataset then I strongly suggest that you use them to show a spinner as I did in Example 5 (see them all here). For a complete list of everything that got fixed and improved take a look at the full list of changes/fixes/enhancements (there's a lot) in the PR #336 Also for your info, the GitHub demo page always gets rebuild anytime a PR is merged and so Example 5 already as the new code including the 25k row button to test it out. Give it a try, I'm done with it I have a couple of other things that I want to look into, which are unrelated to Tree Data, so I'm not expecting to release a new version until at least next week. |
wow,excellent work. in my pc, for example 05: 25k is nice, 50k: 3s, 100k:>=10s for every sort. there should be something can be improved, but it should be another issue. |
Well this is it, I fixed whatever I wanted to fix and I need to focus on other things, so I'm officially done with it (I'm currently doing a project now, well 3 different teams actually, with Tree Data which is also why I did a few other enhancements and fixes and I think I covered everything I had to cover). oh and I also created a new Tree Data - Wiki today. |
it took some time because I worked on other features and it is now release, see the new version 0.14.x |
I'm submitting a Bug report
Your Environment
git current master branch
Describe the Bug
Steps to Reproduce
yarn dev:watch
switch to example 5, the Hierarchical tree is show sort by title, not count the tree parent/child relation
click title column two time to switch sort back to ascending, now the tree show is correct with the parent/child relation .
Expected Behavior
the tree should show the parent/child relation on first show.
Current Behavior
the tree not show the parent/child relation on first show.
The text was updated successfully, but these errors were encountered: