Add a hint for indicating that a Tree is topologically sorted #230

EdSchouten · 2022-09-30T08:04:12Z

I'm currently trying to improve the performance of handling of large output directories (Tree messages), having sizes in the order of hundreds of megabytes. In the process, I have realised that there is a lot of value in enforcing that the Directory messages contained in them are topologically sorted. Two practical use cases:

When instantiating the contents of a Tree on a local file system, having the Tree be topologically sorted allows you to immediately create files and directories in the right place.
When needing to resolve the properties of a single file by path, a topologically sorted Tree permits resolution (of paths without ..) by doing a simple forward scan.

Especially when other features like compression are taken into account, it's useful if Tree messages can be processed in a streaming manner.

One practical issue is that most Protobuf libraries don't offer APIs for processing messages in a streaming manner. This means that implementors who want to achieve these optimisations will need to write their own message parsers; at least for the Tree itself. To make this as painless as possible, we also require that the Tree is stored in some normal form.

Fixes: #229

EdSchouten · 2022-09-30T13:23:08Z

Cc @werkt, as he also wanted to make improvements to Tree in #159.

EdSchouten · 2022-10-02T08:06:01Z

Also cc @peterebden, as he made changes in this area as well in #223

build/bazel/remote/execution/v2/remote_execution.proto

EdSchouten · 2022-10-11T14:48:24Z

Thanks everyone for your time discussing this proposal today. I've made the change suggested by @sstriker, documenting that manual construction of Tree objects without using the Protobuf library is recommended.

mostynb

You mentioned that this implies a limit of 16(?) possible fields in this message- I think it might be worth mentioning that so we don't forget about it. Otherwise this lgtm.

EdSchouten · 2022-10-13T07:35:31Z

You mentioned that this implies a limit of 16(?) possible fields in this message- I think it might be worth mentioning that so we don't forget about it. Otherwise this lgtm.

I think this was @bergsieker asking whether or not we should require that the tag is encoded as a single byte. In my opinion this is acceptable, for the following reason.

The tag is essentially just a variable length integer, having value (fieldNumber << 3) | fieldType. In my proposal I'm stating that the field number can only be 1 or 2, with no room for future extensions. This means that the tag can only be a 5-bit number, which can always be encoded in a single byte.

I have just extended the phrasing to clarify this.

I'm currently trying to improve the performance of handling of large output directories (Tree messages), having sizes in the order of hundreds of megabytes. In the process, I have realised that there is a lot of value in enforcing that the Directory messages contained in them are topologically sorted. Two practical use cases: - When instantiating the contents of a Tree on a local file system, having the Tree be topologically sorted allows you to immediately create files and directories in the right place. - When needing to resolve the properties of a single file by path, a topologically sorted Tree permits resolution by doing a simple forward scan. Especially when other features like compression are taken into account, it's useful if Tree messages can be processed in a streaming manner. One practical issue is that most Protobuf libraries don't offer APIs for processing messages in a streaming manner. This means that implementors who want to achive these optimisations will need to write their own message parsers; at least for the Tree tree itself. To make this as painless as possible, we also require that the Tree is stored in some normal form. Fixes: bazelbuild#229

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230

EdSchouten · 2022-10-13T11:24:31Z

Draft PR against Bazel to start generating topologically sorted trees: bazelbuild/bazel#16463

I will mark it ready for review as soon as this PR lands.

I have submitted the following PR against remote-apis, which adds a hint to ActionResult to indicate that the Directory messages contained in a Tree are topologically sorted: bazelbuild/remote-apis#230 The advantage of having Tree objects in this form is that it makes it possible by doing instantiation of Tree objects on a local file system using a simple forward scan. As this change hasn't been merged yet, this comment only adds the logic for generating topologically sorted Tree objects. I will only add the code for announcing the hint as soon as the linked PR gets merged.

EdSchouten · 2022-10-17T07:46:49Z

@bergsieker Would you be interested in merging this PR for me? Thanks!

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230 Partial commit for third_party/*, see #16463. Signed-off-by: Sunil Gowroji <[email protected]>

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230 Closes #16463. PiperOrigin-RevId: 487196375 Change-Id: Iafcfd617fc101fec7bfa943552113ce57ab8041b

remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230 Partial commit for third_party/*, see #16463. Signed-off-by: Sunil Gowroji <[email protected]>

* Emit Tree objects in topological order remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230 Closes #16463. PiperOrigin-RevId: 487196375 Change-Id: Iafcfd617fc101fec7bfa943552113ce57ab8041b * Emit Tree objects in topological order remote-apis PR 230 added a way where producers of Tree messages can indicate that the directories contained within are stored in topological order. The advantage of using such an ordering is that it permits instantiation of such objects onto a local file system in a streaming fashion. The same holds for lookups of individual paths. Even though Bazel currently does not gain from this, this change at least modifies Bazel's REv2 client to emit topologically sorted trees. This makes it possible for tools such as Buildbarn's bb-browser to process them more efficiently. More details: - bazelbuild/remote-apis#229 - bazelbuild/remote-apis#230 Partial commit for third_party/*, see #16463. Signed-off-by: Sunil Gowroji <[email protected]> Signed-off-by: Sunil Gowroji <[email protected]> Co-authored-by: Ed Schouten <[email protected]>

I have submitted the following PR against remote-apis, which adds a hint to ActionResult to indicate that the Directory messages contained in a Tree are topologically sorted: bazelbuild/remote-apis#230 The advantage of having Tree objects in this form is that it makes it possible by doing instantiation of Tree objects on a local file system using a simple forward scan. As this change hasn't been merged yet, this comment only adds the logic for generating topologically sorted Tree objects. I will only add the code for announcing the hint as soon as the linked PR gets merged.

Regenerate the Go source code for the Remote Execution protocol

a58a75a

EdSchouten requested review from buchgr and bergsieker as code owners September 30, 2022 08:04

EdSchouten force-pushed the eschouten/20220930-toposort branch 2 times, most recently from dac402b to 0266a4b Compare September 30, 2022 08:11

EdSchouten force-pushed the eschouten/20220930-toposort branch from 0266a4b to e81e679 Compare October 1, 2022 16:25

mostynb reviewed Oct 2, 2022

View reviewed changes

build/bazel/remote/execution/v2/remote_execution.proto Outdated Show resolved Hide resolved

EdSchouten force-pushed the eschouten/20220930-toposort branch 2 times, most recently from 01a52cc to cd66598 Compare October 2, 2022 17:42

EdSchouten force-pushed the eschouten/20220930-toposort branch from cd66598 to e37ee76 Compare October 11, 2022 14:47

mostynb approved these changes Oct 12, 2022

View reviewed changes

EdSchouten force-pushed the eschouten/20220930-toposort branch from e37ee76 to c3e4d01 Compare October 13, 2022 07:35

EdSchouten mentioned this pull request Oct 13, 2022

Emit Tree objects in topological order bazelbuild/bazel#16463

Closed

EdSchouten requested a review from EricBurnett October 19, 2022 11:32

bergsieker removed the request for review from buchgr October 21, 2022 20:39

bergsieker approved these changes Oct 21, 2022

View reviewed changes

bergsieker merged commit f608918 into bazelbuild:main Oct 21, 2022

EdSchouten deleted the eschouten/20220930-toposort branch October 23, 2022 02:14

EdSchouten mentioned this pull request Oct 23, 2022

REv3 idea: Make is_topologically_sorted the default, and eliminate tag bytes #232

Open

ShreeM01 mentioned this pull request Dec 1, 2022

[6.0.0] Emit Tree objects in topological order bazelbuild/bazel#16903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a hint for indicating that a Tree is topologically sorted #230

Add a hint for indicating that a Tree is topologically sorted #230

EdSchouten commented Sep 30, 2022 •

edited

Loading

EdSchouten commented Sep 30, 2022

EdSchouten commented Oct 2, 2022

EdSchouten commented Oct 11, 2022

mostynb left a comment

EdSchouten commented Oct 13, 2022

EdSchouten commented Oct 13, 2022

EdSchouten commented Oct 17, 2022

Add a hint for indicating that a Tree is topologically sorted #230

Add a hint for indicating that a Tree is topologically sorted #230

Conversation

EdSchouten commented Sep 30, 2022 • edited Loading

EdSchouten commented Sep 30, 2022

EdSchouten commented Oct 2, 2022

EdSchouten commented Oct 11, 2022

mostynb left a comment

Choose a reason for hiding this comment

EdSchouten commented Oct 13, 2022

EdSchouten commented Oct 13, 2022

EdSchouten commented Oct 17, 2022

EdSchouten commented Sep 30, 2022 •

edited

Loading