Use shared Zstd(De)Compressor in ResolvedFunction encoding

[wendigo]

Description

Speeds up ResolvedFunction serialization and deserialization, i.e. TestLocalExecutionPlanner now executes in 55 30 seconds, instead of 2 minutes 33 seconds.

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core query engine

How would you describe this change to a non-technical end user or system administrator?

Speeds up function resolution

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[electrum]

ZstdDecompressor is not safe to share across threads. It uses ZstdFrameDecompressor which holds state during the decompression operation.

ZstdCompressor has no state, so there shouldn't be any benefit to reusing it.

We don't document the thread safety of the Airlift Compressor and Decompressor interfaces, but probably should. cc @martint

[wendigo]

@electrum actually ByteBuffer based methods are thread safe in ZstdDecompressor

[electrum]

@wendigo you're right. That's a strange choice. We should probably make them both thread safe.

In that case, we need to order the commits so that the byte buffer change is first, so that the commit which uses a shared decompressor is safe.

[wendigo]

@electrum airlift/aircompressor#139

[martint]

We don't document the thread safety of the Airlift Compressor and Decompressor interfaces, but probably should. cc

Compressors and Decompressors are not meant to be thread safe. Some may be due to implementation details, but it's not a requirement. Yes, we should document that.

[martint]

actually ByteBuffer based methods are thread safe in ZstdDecompressor

That's only a side effect -- we had to resort to synchronization to prevent the JVM from collecting the objects (and deallocating the underlying buffers) while the method is running. But the methods are not required or guaranteed to be thread safe (as mentioned above, Compressors and Decompressors are not meant to be thread-safe)

[findepi]

is the checks' failure related?

[wendigo]

@findepi I don't think so. There is a problem with upper bounds for dbpool from commons in redis module. I'm investigating.

[wendigo]

@findepi #11462 this will remove the version conflict in this PR.

[findepi]

@findepi I don't think so. There is a problem with upper bounds for dbpool from commons in redis module. I'm investigating.

Do you mean we don't have precise version pinned?
that would mean the build is not reproducible...

[wendigo]

@findepi there was dependency pulled in by redis module (transitive from jedis client) and I've added another version since the conflict.

[wendigo]

@findepi PTAL now. I've reworked this PR

[findepi]

"Reuse thread-local Zstd de/compressor in ResolvedFunction to Qualifie…"

[findepi]

"Reuse thread-local Zstd de/compressor in ResolvedFunction to Qualifie…"

[findepi]

"Speed up QualifiedName implementation"

[findepi]

"Memoize ResolvedFunction <-> QualifiedName conversions"

[findepi]

@dain PTAL

[wendigo]

JMH results: https://jmh.morethan.io/?gists=d2ceb76e826da7295d86efce0088a550,d77deb7f53c44b9391a725138075dae7

[findepi]

thanks @wendigo for benchmark results

@martint @electrum you both requested changes on this PR. Do you actually plan on reviewing it?

stale

[wendigo]

@findepi can we merge it?

[findepi]

@findepi can we merge it?

That's probably a question to David.