[FIXED] Meta snapshot performance & missing consumers

[MauriceVanVeen]

Due to the custom marshalling offline asset awareness made the meta snapshot take a longer time to compute, even if there are no offline assets. This PR improves that by performing the meta snapshot as before if there are no offline assets, and only have those offline assets go through the slower path.

Also fixes a bug where consumers would be lost when a new meta snapshot was created for an unsupported stream.

Signed-off-by: Maurice van Veen github@mauricevanveen.com

[wallyqs]

can you take a look at adapting TestNoRaceJetStreamClusterLargeMetaSnapshotTiming to have a baseline here? Test with and without offline assets for example

[MauriceVanVeen]

can you take a look at adapting TestNoRaceJetStreamClusterLargeMetaSnapshotTiming to have a baseline here? Test with and without offline assets for example

The test is highly dependent on hardware as to how long it runs (it generally runs slower on my machine than in the other examples). So IMO it would be better to have separate benchmarking, instead of doing this in a unit test here.

[wallyqs]

@MauriceVanVeen you can have a different version of the test with a smaller number of streams and assets as a quick test, then measure that the time processing without offline assets is faster than with offline assets, besides the test for benchmarking where we also need to consider different configurations (number of streams, consumers...)

[neilalexander]

*consumerAssignment is pointerised here but backingStreamAssignment isn't on the other side. Is that meant to be the case?

[MauriceVanVeen]

Discussed this also offline, but this was indeed intentional to not need to use *ca in &writeableConsumerAssignment{consumerAssignment: ca}.
Have tested if there's a difference between not having this take a pointer, but there seems no significant difference.
(Actually the current approach seems faster, but hard to tell because the test varies greatly in performance numbers already on re-run, so it might just be my laptop running less hot then)

[neilalexander]

I'm not convinced that a benchmark that just compares the supported vs unsupported path is going to be meaningful, we know that the unsupported path is slower, a test isn't going to tell us anything new about that.

I think it probably makes far more sense that we add a benchmark to the Gauge set that creates a bunch of assets and then measures the amount of time it takes to call JetStreamSnapshotMeta() however many times, both with and without unsupported state, so that we can follow the trends across releases. That is normally where we look first for meaningful changes in performance.

[MauriceVanVeen]

Temporarily on draft, to do some additional testing for an edge case.

[wallyqs]

The gauge bench sounds good too though I think a simple benchmark in the repo should be good enough at least for coverage of the changes in the PR, does not have to be too complicated.

[neilalexander]

LGTM