wireproto: support for pullbundles
Needs ReviewPublic

Authored by joerg.sonnenberger on Fri, Jan 12, 12:32 PM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Summary

Pullbundles are similar to clonebundles, but served as normal inline
bundle streams. They are almost transparent to the client -- the only
visible effect is that the client might get less changes than what it
asked for, i.e. not all requested head revisions are provided.

The client announces support for the necessary retries with the
partial-pull capability. After receiving a partial bundle, it updates
the set of revisions shared with the server and drops all now-known
heads from the request list. It will then rerun getbundle until
no changes are received or all remote heads are present.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

For my test case, which is a bundle of all changes in the NetBSD repo before 2014 and a yearly bundle afterwards until 2018/1/1 and normal pull for the rest, find_pullbundle needs less than 0.5s of CPU time in this iteration when it matches.
After the initial clone and with additional available changes, pull time is:

With pullbundles.manifest: 0.42s
Without pullbundles.manifest: 0.41s

i.e. the difference is the noise level. Further benchmarks by others would be appreciated.

The biggest remaining question for me is whether we want to introduce a capability for the client to mark that it is willing to do multiple pull rounds.

indygreg requested changes to this revision.Sun, Jan 14, 5:42 PM
indygreg added subscribers: durin42, indygreg.

This patch needs a lot of work. But I'm very supportive of the feature and the preliminary implementation!

I wish we could find a way to send multiple, inline, pre-generated bundles in one response. However, the existing design of compression within the bundle2 format doesn't easily facilitate this. We should think long and hard about whether to implement this feature as partial pull or extend bundle2 to allow us to do nice things and avoid the extra client-server roundtrips.

Speaking of compression, we probably need some more code around e.g. the mismatch between on-disk compression and wire compression. There is a potential for local bundles to be stored uncompressed and for the server to compress this data as part of streaming it out. That involves CPU. We still come out ahead because it avoids having to generate the bundle content from revlogs. But it isn't as ideal as streaming an already compressed bundle over the wire. We may want to open the bundle2 files and look at their compression info and make sure we don't do stupid things. It should be fast to parse the bundle2 header to obtain this knowledge.

My biggest concern with the architecture of this feature is the multiple roundtrips. I really wish we could stream multiple bundles off disk to the wire with no decompression/compression involved. That would mean storing compressed bundles on disk. But this would require some additional bundle2 magic. The existing solution is simple and elegant. I do like that. I'd very much like to get the opinion of someone like @durin42 (who also likes designing protocols).

mercurial/wireproto.py
834

This function needs a docstring. And the expected behavior of this function needs to be documented in the docstring and/or inline as comments.

836

: as a delimiter for nodes doesn't feel right because : has meaning for revsets. I would use , or ;.

That being said, it may make the manifest format slightly less human friendly because URI encoding may come into play. That should be fine though: we already escape values for BUNDLESPEC when using packed1 bundles.

838

I think there should be an if not manifest: return here to handle the common case.

848

We'll need documentation of the fields in the manifest somewhere. If we combine this feature with the clonebundles.py extension, that seems like the logical place to document things.

854–862

I worry about performance issues here. You were making some noise about this on IRC. So it sounds like it is on your radar.

I'm just not sure how to make things more efficient. I just know that doing so many DAG tests feels like it will lead to performance problems for repos with hundreds of thousands or millions of changesets.

But we shouldn't let performance issues derail this feature. Pull bundles have the potential to offload tens of seconds of CPU time from the server. So even if DAG operations consume a few seconds of CPU, we come out ahead. It would be nice to get that down to 100's of milliseconds at most though. But this feels like the territory of follow-up work, possibly involving caching or more efficient stores of which revisions are in which bundles.

874–877

Using a URL field for a vfs path (which doesn't have nor handle protocol schemes - e.g. file:// IIRC) feels wrong. I think the entry in the manifest should be PATH`.

928

I'd consider enabling this feature by default. Checking for the presence of a pullbundles.manifest should be a cheap operation. Especially when you consider that serving a bundle will open likely dozens of revlog files.

Another idea to consider is to tie this feature into the clonebundles.py extension. We already have extensive documentation in that extension for offloading server load via pre-generated bundles. I view this feature as a close sibling. I think they could live under the same umbrella. But because "pull bundles" are part of the getbundle wire protocol command, the bulk of the server code needs to remain in core.

929

AFAICT this capability isn't defined anywhere. Is there a missing patch in this series?

I do agree with the use of a partial-pull capability rather than exposing pull bundles functionality in the capabilities layer. Pull bundles should be purely a server-side implementation detail. It's partial pull that is the generic feature that should be exposed as part of the wire protocol.

934

prefer_uncompressed doesn't exist.

I know you were talking about compression behavior on IRC. Please do invent whatever APIs you need to communicate to the wire protocol layer that you want to stream data with whatever compression behavior that you need.

This revision now requires changes to proceed.Sun, Jan 14, 5:42 PM

Another idea to consider is storing changegroup data or bundle2 parts on disk instead of full bundles. Then, we could stream multiple changegroup parts into a larger bundle2 payload.

That does mean that'd we'd have to compress outgoing data over the wire. So not ideal if minimizing CPU usage is your goal. But we still come out ahead by not having to generate the changegroup data.

This would require a mechanism to generate files of that format, however. Not a huge amount of work. But still work.

I'm just not sure what the sweet spot for this feature is. Is it enough to eliminate changegroup generation overhead. Or are we striving for ~0 CPU usage to service hg pull operations (like what clone bundles achieve)?

I wish we could find a way to send multiple, inline, pre-generated bundles in one response. However, the existing design of compression within the bundle2 format doesn't easily facilitate this. We should think long and hard about whether to implement this feature as partial pull or extend bundle2 to allow us to do nice things and avoid the extra client-server roundtrips.

I was looking at both parts initially. The existing bundle2 compression doesn't work very well for this purpose as it doesn't allow switching compression in the middle. It might be possible to hack something together with a flag to hg bundle to skip the final zero record and depend on the ability of all currently supported compression engines to do concat streams, but that feels like a crude hack. Doing extra client-server roundtrips is a bit annoying and costly, but it also makes the functionality a bit more forgiving for mistakes in the bundle specification. Unlike clonebundles, it requires the records to be much more precise and we certainly don't want to parse the bundles themselve over and over again. It might be nice for some future bundle version to allow easy access to the roots and leaves of the DAG.

Speaking of compression, we probably need some more code around e.g. the mismatch between on-disk compression and wire compression. There is a potential for local bundles to be stored uncompressed and for the server to compress this data as part of streaming it out. That involves CPU. We still come out ahead because it avoids having to generate the bundle content from revlogs. But it isn't as ideal as streaming an already compressed bundle over the wire. We may want to open the bundle2 files and look at their compression info and make sure we don't do stupid things. It should be fast to parse the bundle2 header to obtain this knowledge.

The bundle will be sent without wire compression if supported. A v1 HTTP client will still get the basic zlib framing, but that's not really avoidable. Otherwise, BUNDLESPEC is supposed to make sure that the client knows how to deal with the bundle. The idea is explicitly that the administrator has a free choice over the compression format and the data is send as is. Only crypto overhead should apply.

mercurial/wireproto.py
854–862

As I wrote in the comments on the client side, it seems to be OK now. The heavy lifting is the initial computation of the ancestor sets outside the loop, which is in the worst case an enumeration of all local revisions. That by itself doesn't seem to result in a big performance difference. The rest of the loop depends mostly on the number of roots and leaves of the bundle DAGs. My test case had a total of ~800 and the impact on normal pull operations was in the noise.

874–877

The URL field is synthesized by parseclonebundlesmanifest for the first entry. It isn't tagged in the actual file. As long as the format of the files is otherwise the same, I'd just keep it identical.

928

The bigger question is whether it should have an associated bundle or protocol capability. The client needs to be able to deal with the partial replies. If we go that way, I agree that it can and should be enabled by default.

The clonebundles extension is mostly for show nowadays, the interesting stuff is all in core.

934

Sorry, this was a rebase leak.

joerg.sonnenberger marked 5 inline comments as done.Sun, Jan 14, 7:03 PM

My biggest concern with the architecture of this feature is the multiple roundtrips. I really wish we could stream multiple bundles off disk to the wire with no decompression/compression involved. That would mean storing compressed bundles on disk. But this would require some additional bundle2 magic. The existing solution is simple and elegant. I do like that. I'd very much like to get the opinion of someone like @durin42 (who also likes designing protocols).

I don't disagree with this goal (fewer roundtrips), but I think it's probably better than nothing to do it with multiple roundtrips.

I'm too rusty on bundle2 at the moment to grok what magic would be required to pre-compress payloads.

joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger retitled this revision from wireproto: server-side support for pullbundles to wireproto: support for pullbundles.

I'm too rusty on bundle2 at the moment to grok what magic would be required to pre-compress payloads.

The ideal solution would be a way to reset the context for the byte stream. Essentially we'd add a marker telling consumers they've reached EOF of either a bundle2 stream or a compression context. The next byte should be interpreted as a new bundle2 stream or a new compression context.

In the case of zstandard, we can force the end of a zstandard frame and start sending a new frame. As long as the consumer reads all frames as one coherent stream, the output of the decompressor will look as if nothing special happened. I'm not actually sure if python-zstandard supports this though. But it could. For other compression formats, the answer isn't as clear cut.

It's probably best to have an explicit marker in the bundle2 or protocol stream identifying the end of a compression context. Either a "reset at end of part" flag in the part header. Or an explicit bundle2 part that says "reset after me." Either way, we'd need to invent an API on the compression engine that allows the compression context to be reset. This code could get gnarly, as there are various buffers in play. It's possible. But a bit of work. Since we're at code freeze and it looks like this feature will have to wait for 4.6, it looks like we'll have time to implement this feature if we want to go that route. I don't like scope bloating though.

I'm too rusty on bundle2 at the moment to grok what magic would be required to pre-compress payloads.

The ideal solution would be a way to reset the context for the byte stream. Essentially we'd add a marker telling consumers they've reached EOF of either a bundle2 stream or a compression context. The next byte should be interpreted as a new bundle2 stream or a new compression context.

I should add that if clients automatically consumed all bundle2 payloads sent by the server and the compression context was automatically reset after end of bundle2 payload, then the easiest path forward is likely having the client advertise that it can receive multiple bundle2 payloads and then we implement this feature in terms of sending multiple bundle2 payloads. As long as the compression context is flushed, we should be OK. But we model the response stream as a single stream and feed that whole thing into a compressor/decompressor. We'd need to hack up the compression code a bit no matter what path we use.

joerg.sonnenberger edited the summary of this revision. (Show Details)