Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sql): introduce global select query cache per interface #4525

Merged
merged 22 commits into from
May 22, 2024

Conversation

puzpuzpuz
Copy link
Contributor

@puzpuzpuz puzpuzpuz commented May 20, 2024

Swaps per-connection SELECT query caches with a global per-interface cache (HTTP/PGWire). The cache is a striped version of our existing thread-unsafe cache (AssociativeCache class). The goal is to make sure to utilize hash table stats introduced in #4517 even if the same query is run on different connections.

Microbenchmarks

i7-1185G7 CPU (4c/8t), OpenJDK 17, Ubuntu 22.04:

Benchmark                                                       Mode  Cnt     Score      Error  Units
AssociativeCacheBenchmark.testConcurrentNoMetrics_randomKeys    avgt    3   453.786 ± 1431.665  ns/op
AssociativeCacheBenchmark.testConcurrentNoMetrics_sameKey       avgt    3  2917.800 ±  296.223  ns/op
AssociativeCacheBenchmark.testConcurrentWithMetrics_randomKeys  avgt    3   709.276 ±   16.656  ns/op
AssociativeCacheBenchmark.testConcurrentWithMetrics_sameKey     avgt    3  3038.937 ±  747.867  ns/op
AssociativeCacheBenchmark.testSimpleWithMetrics_randomKeys      avgt    3   153.023 ±   11.507  ns/op
AssociativeCacheBenchmark.testSimpleWithMetrics_sameKey         avgt    3    48.281 ±    3.490  ns/op

Note: metrics on their own have a higher cost than the global cache in the contented case.

@puzpuzpuz puzpuzpuz added Enhancement Enhance existing functionality SQL Issues or changes relating to SQL execution Postgres Wire Issues or changes relating to Postres wire protocol REST API Issues or changes relating to the HTTP endpoints labels May 20, 2024
@puzpuzpuz puzpuzpuz self-assigned this May 20, 2024
@puzpuzpuz puzpuzpuz marked this pull request as draft May 20, 2024 16:30
@puzpuzpuz puzpuzpuz marked this pull request as ready for review May 21, 2024 07:59
@jerrinot jerrinot self-requested a review May 21, 2024 08:17
jerrinot
jerrinot previously approved these changes May 21, 2024
Copy link
Contributor

@jerrinot jerrinot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's looking good!
defaults are sane.
row-shifting while holding a row lock looks unusual, but I don't expect it to cause troubles in practice. in any case - please attach Clickbench numbers before merging it - just to be sure.

@puzpuzpuz
Copy link
Contributor Author

puzpuzpuz commented May 21, 2024

please attach Clickbench numbers before merging it - just to be sure.

That's what I got (note: no regressions in individual queries):
Screenshot from 2024-05-21 16-16-05

row-shifting while holding a row lock looks unusual, but I don't expect it to cause troubles in practice.

System#arraycopy() is pretty fast, so using it to enforce FIFO eviction seems reasonable to me. I didn't observe a big difference with the code that didn't guarantee FIFO and tried to insert objects in-place.

@puzpuzpuz
Copy link
Contributor Author

@jerrinot thanks for the review!

Copy link
Member

@bluestreak01 bluestreak01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the concurrent cache is on critical path, there is no decent fuzz test for it and i don't think we have an abundance of concurrent query tests to exercise the cache fully

@puzpuzpuz
Copy link
Contributor Author

@bluestreak01 I've added an E2E stress test in 5c75de9

@ideoma
Copy link
Collaborator

ideoma commented May 22, 2024

[PR Coverage check]

😍 pass : 275 / 283 (97.17%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/engine/functions/test/TestOwnerCountingFunctionFactory.java 32 36 88.89%
🔵 io/questdb/std/ConcurrentAssociativeCache.java 73 76 96.05%
🔵 io/questdb/std/SimpleAssociativeCache.java 67 68 98.53%
🔵 io/questdb/std/NoOpAssociativeCache.java 6 6 100.00%
🔵 io/questdb/PropServerConfiguration.java 10 10 100.00%
🔵 io/questdb/cutlass/pgwire/PGWireServer.java 13 13 100.00%
🔵 io/questdb/cutlass/http/HttpConnectionContext.java 2 2 100.00%
🔵 io/questdb/cutlass/http/HttpServer.java 17 17 100.00%
🔵 io/questdb/cutlass/pgwire/DefaultPGWireConfiguration.java 1 1 100.00%
🔵 io/questdb/cutlass/Services.java 3 3 100.00%
🔵 io/questdb/cutlass/pgwire/AbstractTypeContainer.java 6 6 100.00%
🔵 io/questdb/cutlass/pgwire/TypesAndSelect.java 7 7 100.00%
🔵 io/questdb/cutlass/http/DefaultHttpServerConfiguration.java 2 2 100.00%
🔵 io/questdb/cutlass/pgwire/PGConnectionContext.java 36 36 100.00%

@bluestreak01 bluestreak01 merged commit 3729112 into master May 22, 2024
24 checks passed
@bluestreak01 bluestreak01 deleted the puzpuzpuz_global_select_cache branch May 22, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhance existing functionality Postgres Wire Issues or changes relating to Postres wire protocol REST API Issues or changes relating to the HTTP endpoints SQL Issues or changes relating to SQL execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants