Put nodes from Directory->getChildren in the ObjectTree cache

[icewind1991]

Saves us from having to generate the node again later in getNodeForPath for a significant performance improvement when working with large folders

Also note that creating the node in getChildren is cheaper than in getNodeForPath since in the former we already have the fileinfo

comparison

cc @DeepDiver1975 @rullzer

[mention-bot]

By analyzing the blame information on this pull request, we identified @DeepDiver1975, @PVince81 and @LukasReschke to be potential reviewers

[DeepDiver1975]

Afaik. Nodes are already cached within the sabre code ... Will have a closer look.

[PVince81]

Yes, Sabre already cached objects when calling getNodeForPath: https://github.com/owncloud/3rdparty/blob/master/sabre/dav/lib/DAV/Tree.php#L80

@icewind1991 did you identify code paths where it's not the case ?
Also note that Sabre 3.1 has a fix where caching wasn't always done properly: https://github.com/fruux/sabre-dav/pull/729 which @rullzer fixed

[rullzer]

@PVince81 I think that patch is also in the 3.0 branch. So we should see effect already.

@icewind1991 can you desribe the scenario where you don't hit the cache... seems better to fix upstream.

[icewind1991]

While handling a depth=1 propfind we end up doing a getChildren on the folder node to load all the fileinfo, later sabredav iterates trough the children using the ObjectTree.

The ObjectTree does proper caching but the result from Node->getChildren is not put into the cache.

[rullzer]

Ah! now I get it. It is because we get the nodes ourself instead of letting sabre handle it all...

[rullzer]

It feels like we duplicate a bit to much of the code from sabre in general in the object tree..
But this indeed makes the caching work.

👍

[rullzer]

@evert would it maybe make sense to get this upstream into sabre? Might also be usefull for other projects that extend the Tree

[DeepDiver1975]

Might also be usefull for other projects that extend the Tree

in an ideal world we should not extend the tree ....

[evert]

I don't think this is possible in sabre/dav, because the standard node does not know it's own path. Furthermore, Nodes are not aware of the Tree. I would not want this circular dependency.

If you call Tree::getChildren() the result does get cached though.

[DeepDiver1975]

The ObjectTree does proper caching but the result from Node->getChildren is not put into the cache.

so we better iterate using the object tree instead of calling node->getchildren ?

[rullzer]

so we better iterate using the object tree instead of calling node->getchildren ?

Yes, that would make things cleaner I think.
But I think getting this is for now is a good performance improvement before we start refactoring all of our webdav stuff ;)

[LukasReschke]

👍