[zstd] pass level param through to compress/zstd encoder

[lizthegrey]

I'm also open to explicitly enumerating the encoder categories as was done with gzip, but decided to do the sync.Map approach instead for simplicity in case the number range changes meaning down the road and we need more or fewer numbers, etc.

https://pkg.go.dev/sync#Map says it's okay to use it as it fits (1) the entry for a given key is only ever written once but read many times, as in caches that only grow

[lizthegrey]

closing/reopening to possibly get CLA status to refresh? there we go.

[dnwe]

@lizthegrey thanks for these changes, I'm happy to approve as I think we should be exposing the level

FWIW we previously had a PR from someone (#1869) suggesting that sync.Once locking was somehow expensive and that's why the setup had been moved to func init(), but we probably should have pressed for more benchmark evidence at that time because it seems like it would only ever have effected startup time and wasn't worth optimising for.

Aside, whilst it doesn't strictly need it at the moment (because we're not exposing any options), could you mirror the use of a sync.Map in the decode case too so that the decoder is only initialised on-demand in a similar way? I know people have been interesting in customising the encoder and decoder so I think this would be a good step toward exposing more options going forward.

[lizthegrey]

Aside, whilst it doesn't strictly need it at the moment (because we're not exposing any options), could you mirror the use of a sync.Map in the decode case too so that the decoder is only initialised on-demand in a similar way? I know people have been interesting in customising the encoder and decoder so I think this would be a good step toward exposing more options going forward.

Do you prefer use of a Pool or of a single returned zstd.Reader? I worry there's no meaningful map key to use, so might the Pool be a better construct to use here?

[lizthegrey]

Aside, whilst it doesn't strictly need it at the moment (because we're not exposing any options), could you mirror the use of a sync.Map in the decode case too so that the decoder is only initialised on-demand in a similar way? I know people have been interesting in customising the encoder and decoder so I think this would be a good step toward exposing more options going forward.

Do you prefer use of a Pool or of a single returned zstd.Reader? I worry there's no meaningful map key to use, so might the Pool be a better construct to use here?

Or we could create a CompressionOptions{} struct that could be used as the map key I suppose. still leaves open why the other mechanisms use a Pool but zstd does not.

[dnwe]

@lizthegrey historically this was due to the use of the klaus/compress library for zstd which differs from compress/gzip and /pierrec/lz4, because it manages its own sync.Pool of encoders/decoders and spawns goroutines when you invoke it (see compress#264 for some of the old context)

[lizthegrey]

@lizthegrey thanks for these changes, I'm happy to approve as I think we should be exposing the level

FWIW we previously had a PR from someone (#1869) suggesting that sync.Once locking was somehow expensive and that's why the setup had been moved to func init(), but we probably should have pressed for more benchmark evidence at that time because it seems like it would only ever have effected startup time and wasn't worth optimising for.

Aside, whilst it doesn't strictly need it at the moment (because we're not exposing any options), could you mirror the use of a sync.Map in the decode case too so that the decoder is only initialised on-demand in a similar way? I know people have been interesting in customising the encoder and decoder so I think this would be a good step toward exposing more options going forward.

Done, there's now both ZstdDecoderParams and ZstdEncoderParams and lazy-creation when a decoder or encoder is requested with a new set of parameters.

[dnwe]

Perfect, thank you!