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

[tree-table] Refactor tree-table, make data structures private #500

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 5, 2018

Refactors the tree-table component to use private internal data
structures instead of the semi-public LinkedListTree. Replaces the
LinkedListTree with a simpler CollapseTree which should be much cheaper
to build and operate in general.

The new data structure automatically wraps a much simpler POJO structure
that should be passed into TreeTable. It will respond to changes in the
underlying data structure, so if the user updates the POJO everything
will update accordingly.

Refactors the tree-table component to use private internal data
structures instead of the semi-public LinkedListTree. Replaces the
LinkedListTree with a simpler CollapseTree which should be much cheaper
to build and operate in general.

The new data structure automatically wraps a much simpler POJO structure
that should be passed into TreeTable. It will respond to changes in the
underlying data structure, so if the user updates the POJO everything
will update accordingly.
@visopsys
Copy link

visopsys commented Apr 5, 2018

This is an interesting data structure. Some question I want to check if my understanding is correct:

  1. When you collapse a node, you change the node's length. The parent offset list needs to be updated. If the parent has N children, this will be an O(N) operation. Does this mean we will have O(N) operation for collapsing/expanding?

  2. objectAt is a function that calls closestLessThan which is an O(logN) operation. Does this mean dragging a scroller quickly will require O(NlogN) operation?

This might not be a big issue until the table hits 100k rows. If the API is better, we can adopt this over LinkedListTree

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 5, 2018

  1. This is true, but will be paid only once during an operation which is relatively uncommon (collapsing/uncollapsing is a user driven event). This is better than the current situation, which is O(N) on every scroll potentially - in the good case it's fine, but in the bad case it's really very bad.
  2. It will actually be O(M logN) where M is the number of rendered items (VC will not render every intermediate item, even on fast scrolls. It's gated to 1 update per frame). This is generally very small, ~20-50, so logN dominates here and it's still very fast. We can also further optimize this with a "pointer" like strategy - store the location of the last visited node, and traverse up then back down the tree as necessary. That would reduce the time to being similar to the linked list strategy, while still saving on memory.

The main advantage to this strategy besides autowrapping is reducing the number of allocations by an order of magnitude. We generally have trees which are pretty flat, and leveraging this can reduce the number of allocations to a point where it's almost unnoticeable. Let's say you have a tree with 100 children per parent. At 3 levels deep, that would be 1,000,000 rows. Creating a node per actual, real value-node takes a huge amount of time, and costs a massive amount of memory. By comparison, this tree would generate 10,000 rows, and can be done almost instantaneously on a decently powerful machine.

Denser trees will likely not benefit nearly as much from this strategy, but it also shouldn't hurt them. O(M logN) is still very fast, and we can optimize it further if needed.

// Changes to the value directly should properly update all computeds on this
// node, but we need to manually propogate changes upwards to notify any other
// watchers
addObserver(this, 'length', () => Ember.propertyDidChange(parent, 'length'));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this create memleak when Node is no longer used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observers are instance state and will be garbage collected along with the instance, so long as there are no other references to the instance.

@visopsys
Copy link

visopsys commented Apr 5, 2018

:+1

Overall looks good. Only one comment regarding the observer.

@pzuraq pzuraq merged commit f99f19d into master Apr 5, 2018
@pzuraq pzuraq deleted the pzuraq/refactor-tree-table branch April 5, 2018 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants