This repository has been archived by the owner on Jan 3, 2024. It is now read-only.
[experimental] rgw/sfs: don't use storage.open_forever() #201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Given we have multiple threads which independently open the database, I'm not sure having the first connection open forever makes sense or does anything useful. Let's see if it changes the benchmarks. Signed-off-by: Tim Serong <[email protected]>
tserong
force-pushed
the
wip-sfs-dont-open-db-forever
branch
from
87e8663 to
2316024
Compare
|
A bit more context may be in order. I observed that when running s3gw, we end up with two long-lived FDs open, pointing to s3gw.db (ignore the wal and shm lines here, they belong to the first connection): But, when anything interesting happens (e.g.: putting an object, or garbage collection), those things happen in separate threads, which open their own connections to the database, in addition to the above. So, why bother having the initial connection live forever if it's not (or barely) used? |
|
Marking as draft because this is definitely not ready for consumption :) Do we have further information on what happens when we have a lot of in-flight requests? |
A whole lot of db connections get opened and closed separately one after another, in separate threads. AFAICT they don't use the "main" long lived connection at all, which is what this PR gets rid of. I know this because I've watched a bunch of (I opened this PR because Marcel was interested in taking it to see if it had any effect on benchmarks, and I assumed we could revert it afterwards if necessary.) |
Merging and reverting does not /feel/ right. Maybe #202 does what I hope it does - build and push an image for a labeled PR. Once we have that experiments like this become easier :) |
I've done some sketchy instrumentation of sqlite_orm.h, as follows: --- sqlite_orm.h.orig 2023-09-01 15:17:06.513939439 +1000
+++ sqlite_orm.h 2023-09-06 18:34:40.116985427 +1000
@@ -9620,6 +9620,8 @@
#include <string> // std::string
#include <system_error> // std::system_error
+#include <iostream>
+
// #include "error_code.h"
namespace sqlite_orm {
@@ -9630,9 +9632,20 @@
connection_holder(std::string filename_) : filename(move(filename_)) {}
+ void tserong_log_call(const char * fn) {
+ pthread_t me = pthread_self();
+ std::cerr << "thread " << std::hex << me;
+ char name[16];
+ if (0 == pthread_getname_np(me, name, 16)) {
+ std::cerr << " (" << name << ")";
+ }
+ std::cerr << " calling " << fn << std::endl;
+ }
+
void retain() {
++this->_retain_count;
if(1 == this->_retain_count) {
+ tserong_log_call("sqlite3_open");
auto rc = sqlite3_open(this->filename.c_str(), &this->db);
if(rc != SQLITE_OK) {
throw std::system_error(std::error_code(sqlite3_errcode(this->db), get_sqlite_error_category()),
@@ -9644,6 +9657,7 @@
void release() {
--this->_retain_count;
if(0 == this->_retain_count) {
+ tserong_log_call("sqlite3_close");
auto rc = sqlite3_close(this->db);
if(rc != SQLITE_OK) {
throw std::system_error(std::error_code(sqlite3_errcode(this->db), get_sqlite_error_category()),Then, I ran Note: I did NOT have the patch from this PR applied during this first test. Here's what happened:
Here's the raw output in case anyone wants to verify my counting: Repeating the same test with the patch from this PR applied gives:
Here's the raw output from that second run: |
|
Repeating the above two tests, but running under
So either way, the WAL is opened and closed many times, but with the db open forever, the main database file is only opened three times vs. 165 times with this patch applied. |
|
Very interesting experiment. Looking at the
What about disabling the |
OK, that gets really interesting: Running
One of these -- http://localhost:9090/sfs -- results in a single sqlite3_open/sqlite3_close pair, from the "status-server" thread. The others do nothing obvious or interesting. Running WITHOUT
|
They all use the sqlite db pointer created on start and take a copy. They don't renew. If that pointer breaks, sqlite error logs that we called a sqlite3_* function with a broken pointer. |
So the stats thread is somehow behaving differently than the GC thread and the worker(?) threads that handle object puts etc., because the latter all seem to open their own connections. Why take a different approach for the stats thread? |
|
Yeah, I think it's different because GC is accessing the database opening a new connection while the status page is using the raw pointer we get when we do the first connection. |
|
But why? I mean, why does the status page take a copy of the raw pointer, but every other thread makes its own connections? Or, conversely, why does every other thread not take a copy of the raw pointer? |
Just out of curiosity, I ran with
|
irq0
added
ci/build-s3gw-image
|
📦 quay.io/s3gw/s3gw:pr-b5ff3a323b24b3e50a5d91c2ba3a2eeecc22b940-6119665587-1 https://quay.io/repository/s3gw/s3gw?tab=tags&tag=pr-b5ff3a323b24b3e50a5d91c2ba3a2eeecc22b940-6119665587-1 |
Yep, that worked :-) (For anyone else reading this in future, it looks like it landed in #203). Perf looks pretty much the same with/without this (6119665587 refers to this PR): S3GW Performance Report pr201.pdf (Sorry about the PDF, github won't let me upload HTML, which is the original) |
To answer my own question: because the status thread needs to call |
|
TL;DR: we need |
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Sep 13, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Sep 19, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Sep 20, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Sep 20, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Sep 25, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Oct 14, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
tserong
added a commit
to tserong/ceph
that referenced
this pull request
Oct 17, 2023
Currently there is one long lived connection to the sqlite database, but also many other threads use their own connections (for some analysis of this, see aquarist-labs#201). This PR changes all the copies of the storage object to references, which means we're actually only using one db connection now. It's a bit irritating to do this, because it's way too easy to accidentally make a copy if you leave the '&' off :-/ I'd really want to somehow disable copy construction of the Storage object, but I didn't figure out how to do that yet. One interesting effect of this change is that, prior to this commit, the SFS status page only showed SQLite stats for the connection from the status thread, which is not overly helpful. With this commit (all threads using the same connection), the figures from the SQLite stats will actually change over time while s3gw is being used. Note that as we're building with -DSQLITE_THREADSAFE=1, i.e. we're using Serialized mode, it's totally cool to have one connection shared by multiple threads (see https://www.sqlite.org/threadsafe.html) Signed-off-by: Tim Serong <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given we have multiple threads which independently open the database, I'm not sure having the first connection open forever makes sense or does anything useful. Let's see if it changes the benchmarks.