dskit icon indicating copy to clipboard operation
dskit copied to clipboard

Published 20 hours ago • verified publisher icon grafana

Adding GRPC S2 compression implementation

Open deniszh opened this issue 1 year ago • 9 comments

What this PR does:

Adding S2 compression into GRPC encoding.

Which issue(s) this PR fixes:

Fixes #8522

Checklist

  • [x] Tests updated
  • [x] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Implementation mostly taken from https://github.com/mostynb/go-grpc-compression/blob/main/internal/s2/s2.go Also added snappy grpc compression benchmark, to match with S2:

goos: darwin
goarch: arm64
pkg: github.com/grafana/dskit/grpcencoding/s2
cpu: Apple M1
BenchmarkS2SCompress-8             	  249318	      4038 ns/op
BenchmarkS2Decompress-8            	 9593479	       120.8 ns/op
BenchmarkS2GrpcCompressionPerf-8   	    9314	    120118 ns/op
PASS
ok  	github.com/grafana/dskit/grpcencoding/s2	4.409s
goos: darwin
goarch: arm64
pkg: github.com/grafana/dskit/grpcencoding/snappy
cpu: Apple M1
BenchmarkSnappyCompress-8              	  373032	      2785 ns/op
BenchmarkSnappyDecompress-8            	 5234221	       250.9 ns/op
BenchmarkSnappyGrpcCompressionPerf-8   	   12583	     93050 ns/op
PASS
ok  	github.com/grafana/dskit/grpcencoding/snappy	7.214s

deniszh avatar Sep 05 '24 18:09 deniszh

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 05 '24 18:09 CLAassistant

Looks as if you need to run go mod tidy.

aknuds1 avatar Sep 11 '24 13:09 aknuds1

I got some more feedback from @pstibrany, we might want to support S2 directly in Mimir instead of going via dskit (to avoid bloating dskit's dependencies). Could you open a (draft?) PR to Mimir, so we can try to come up with a solution there? Thanks!

aknuds1 avatar Sep 12 '24 13:09 aknuds1

Sure, let me do that

deniszh avatar Sep 12 '24 13:09 deniszh

@aknuds1 : not sure how to do that on Mimir level, will try to do that tomorrow. But btw, which dependencies increase are you talking about? I didn't add new modules, s2 already included in klauspost compress repo. I see only minor upgrade, which IMO should be done anyway.

deniszh avatar Sep 12 '24 21:09 deniszh

not sure how to do that on Mimir level, will try to do that tomorrow.

What @pstibrany suggests is to add a dskit extension point, to allow configuring a custom compression type from Mimir.

If you just open a draft Mimir PR depending on this dskit branch, I'll help you shape it into something acceptable :) It's easier for me to see how to do it when I have a Mimir PR.

aknuds1 avatar Sep 13 '24 07:09 aknuds1

Looks as if you need to run go mod tidy.

Ah, for example app. Just did, together with rebase.

deniszh avatar Sep 13 '24 08:09 deniszh

If you just open a draft Mimir PR depending on this dskit branch, I'll help you shape it into something acceptable :) It's easier for me to see how to do it when I have a Mimir PR.

Sure, I can do that. I didn't do that before because latest Mimir was not compatible with latest dskit, let me try one more time.

I created 2 special branches in dskit for 2.12.0 ans 2.13.0 releases, based on corresponding dskit commits:

  • https://github.com/deniszh/dskit/tree/dzhdanov/grpc-comp-mimir-2.12.0
  • https://github.com/deniszh/dskit/tree/dzhdanov/grpc-comp-mimir-2.13.0 and then created 2 patched branches for Mimir
  • https://github.com/deniszh/mimir/tree/2.12.0-grpc-s2
  • https://github.com/deniszh/mimir/tree/2.13.0-grpc-s2 which just contain repointing Mimir to my dskit.

deniszh avatar Sep 13 '24 08:09 deniszh

Opened draft PR - https://github.com/grafana/mimir/pull/9292

deniszh avatar Sep 13 '24 09:09 deniszh

Opened draft PR - grafana/mimir#9292

merged with https://github.com/grafana/mimir/pull/9322

grooverdan avatar Feb 05 '25 22:02 grooverdan

I still think it can be useful in different projects, but it's only feeling, I didn't test it. So, can be closed.

deniszh avatar Feb 06 '25 05:02 deniszh