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

trees: add diffIdentityProvider for efficient setChildren updates #114237

Merged
merged 7 commits into from
Jan 19, 2021

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Jan 13, 2021

This uses the existing LcsDiff diff implementation to generate
diff operations when calling setChildren on a tree view where the new
diffIdentityProvider is given. I didn't make this 'default' using
the existing identityProvider, since I noticed that (at least) the
Explorer view injects different elements in setChildren that have the
same identity. Previously these would have been replaced, but now they
wouldn't. This may be an effect of compressed trees.

Speaking of compressed trees -- this doesn't work there yet. It looks
like compressed trees call setChildren with the parent of the element
for whom children are being set... this worked before since the update
replaced all previous children, but now the update is not deep if
identities are the same. I haven't dug into this yet to find out why
this is needed.

const node = this.model.getNode(compressedNode) as ITreeNode<ICompressedTreeNode<T>, TFilterData>;
const compressedParentNode = this.model.getParentNodeLocation(compressedNode);
const parent = this.model.getNode(compressedParentNode) as ITreeNode<ICompressedTreeNode<T>, TFilterData>;
const decompressedElement = decompress(node);
const splicedElement = splice(decompressedElement, element, children);
const recompressedElement = (this.enabled ? compress : noCompress)(splicedElement);
const parentChildren = parent.children
.map(child => child === node ? recompressedElement : child);
this._setChildren(parent.element, parentChildren);

Performance numbers. This is much faster for the large test tree.
Previously each update, by the time all tests were discovered, took
about 150ms per tree update:

Now it's an average of 16.3ms per call, and doesn't take longer for
larger trees. Noticably faster to finish and not laggy while it does.

We should consider adopting this for all trees if/when we deal with
the changed setChildren semantics.

This uses the existing `LcsDiff` diff implementation to generate
diff operations when calling `setChildren` on a tree view where the new
`diffIdentityProvider` is given. I didn't make this 'default' using
the existing identityProvider, since I noticed that (at least) the
Explorer view injects different elements in `setChildren` that have the
same identity. Previously these would have been replaced, but now they
wouldn't. This may be an effect of compressed trees.

https://github.com/microsoft/vscode/blob/b6435bc42407d53470e1bc0a0261183148bfde5a/src/vs/base/browser/ui/tree/compressedObjectTreeModel.ts#L151-L162

Speaking of compressed trees -- this doesn't work there yet. It looks
like compressed trees call setChildren with the parent of the element
for whom children are being set... this worked before since the update
replaced all previous children, but now the update is not deep. I
haven't dug into this yet to find out why this is needed.

Performance numbers. This is much faster for the large test tree.
Previously each update, by the time all tests were discovered, took
about 150ms per tree update:

![](https://memes.peet.io/img/21-01-9cbf70b4-6752-4d0a-82b7-f26bd02db01d.png)

Now it's an average of 16.3ms per call, and doesn't take longer for
larger trees. Noticably faster to finish and not laggy while it does.

![](https://memes.peet.io/img/21-01-a8d757b8-0529-4067-9d8c-45138d9db7bc.png)

We should consider adopting this for all trees if/when we deal with
the changed setChildren semantics.
@connor4312 connor4312 self-assigned this Jan 13, 2021
@joaomoreno
Copy link
Member

Very cool @connor4312 👏 This looks fantastic at a glance.

I haven't dug into this yet to find out why this is needed.

Imagine the following compressed tree:

alpha
|- beta
|  |- delta
|  |- gama
|- omega

If one calls setChildren(beta) and removes gama from its children, we should get:

alpha
|- beta/delta
|- omega

But in order to achieve this, we need to change beta into beta/delta, and the only way we have to do that is to setChildren(alpha). In compressed trees, one must always go one level above with setChildren.


I'm getting second thoughts on the API. I wonder if splice (et al.) should instead have options?: { diffEqualsFn: Function } instead of having a long lasting diffIdentityProvider option. That way tree users could control exactly which calls of splice (et al.) could benefit from having the diff computation overhead. What do you think?

@connor4312
Copy link
Member Author

connor4312 commented Jan 13, 2021

I see. I think the simplest solution there would be a diffDeep option (which the compressed tree could set to true automatically) that would caused the index tree to run a diff on children of existing elements to pick up on possible changes there.

If we give the diffing instructions per call then that would be simple to do :)

@connor4312
Copy link
Member Author

Ok -- updated this to take the diff provider in an argument, and an option to specify update recursion levels. Compressed trees will recurse 2 levels in setChildren calls.

@connor4312 connor4312 marked this pull request as ready for review January 13, 2021 21:36
@connor4312
Copy link
Member Author

In a8dd7f6 I noticed that twisties for trees were not updating if they were initially created without children. I think this was hidden by compressed trees due to them recreating from the parent, but this looks like it was a latent bug. I think I recall noticing some cases where twisties were missing, maybe that's what this was...

It doesn't look like anyone outside of the tree ever call setCollapsible, and this just makes it reflect what is the default value for collapsibility at all times .

@connor4312 connor4312 mentioned this pull request Jan 14, 2021
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

I really like this API much better! 👏

src/vs/base/browser/ui/tree/objectTreeModel.ts Outdated Show resolved Hide resolved
@connor4312
Copy link
Member Author

connor4312 commented Jan 19, 2021

Description of changes to reference:

This PR adds two new options, diffIdentityProvider and diffDepth, be passed in an options object as a third argument to tree.setChildren on tree views.

tree.setChildren(
parent,
this.renderNodeList(renderNode, parent === null ? roots() : parent.children),
{ diffIdentityProvider: testIdentityProvider, diffDeep },
);

Previously, calls to setChildren would rerender the entire set of children, even if you only appended one child. This turned out to be quite slow when rendering the test tree with thousands of suites that changed updated several times a second as tests were discovered (#114237 (comment)).

The diffIdentityProvider is an IIdentityProvider that, if provided, will be used to generate a diff of operations such that children are only added or removed. This makes rendering much more efficient, but if you depended setChildren to rerender all elements, it will not do so in this mode.

You can also pass diffDepth to indicate how many levels of children to recurse and diff. It defaults to 0, and can be specified as Infinity.

interface IObjectTreeSetChildrenOptions<T, TFilterData> {
	/**
	 * If set, child updates will recurse the given number of levels even if
	 * items in the splice operation are unchanged. `Infinity` is a valid value.
	 */
	readonly diffDepth?: number;

	/**
	 * Identity provider used to optimize splice() calls in the IndexTree. If
	 * this is not present, optimized splicing is not enabled.
	 *
	 * Warning: if this is present, calls to `setChildren()` will not replace
	 * or update nodes if their identity is the same, even if the elements are
	 * different. For this, you should call `rerender()`.
	 */
	readonly diffIdentityProvider?: IIdentityProvider<T>;

@connor4312 connor4312 merged commit 6815e75 into master Jan 19, 2021
@connor4312 connor4312 deleted the smarter-indexed-setchildren branch January 19, 2021 17:56
@eamodio
Copy link
Contributor

eamodio commented Jan 19, 2021

@connor4312 minor nit -- but I'd suggest calling diffDeep to diffDepth

@connor4312
Copy link
Member Author

Tweaked in eba7c23

@joaomoreno
Copy link
Member

🥳

@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants