-
-
Notifications
You must be signed in to change notification settings - Fork 34.9k
sqlite: optimize column name creation and text value encoding #61954
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| #include "node_errors.h" | ||
| #include "node_mem-inl.h" | ||
| #include "node_url.h" | ||
| #include "simdutf.h" | ||
| #include "sqlite3.h" | ||
| #include "threadpoolwork-inl.h" | ||
| #include "util-inl.h" | ||
|
|
@@ -63,6 +64,20 @@ using v8::TryCatch; | |
| using v8::Uint8Array; | ||
| using v8::Value; | ||
|
|
||
| inline MaybeLocal<String> Utf8StringMaybeOneByte(Isolate* isolate, | ||
| std::string_view input) { | ||
| const int len = static_cast<int>(input.size()); | ||
| if (simdutf::validate_ascii(input.data(), input.size())) { | ||
| return String::NewFromOneByte( | ||
| isolate, | ||
| reinterpret_cast<const uint8_t*>(input.data()), | ||
| NewStringType::kNormal, | ||
| len); | ||
| } | ||
| return String::NewFromUtf8( | ||
| isolate, input.data(), NewStringType::kNormal, len); | ||
| } | ||
|
|
||
| #define CHECK_ERROR_OR_THROW(isolate, db, expr, expected, ret) \ | ||
| do { \ | ||
| int r_ = (expr); \ | ||
|
|
@@ -105,7 +120,10 @@ using v8::Value; | |
| case SQLITE_TEXT: { \ | ||
| const char* v = \ | ||
| reinterpret_cast<const char*>(sqlite3_##from##_text(__VA_ARGS__)); \ | ||
| (result) = String::NewFromUtf8((isolate), v).As<Value>(); \ | ||
| const int v_len = sqlite3_##from##_bytes(__VA_ARGS__); \ | ||
| (result) = \ | ||
| Utf8StringMaybeOneByte((isolate), std::string_view(v, v_len)) \ | ||
| .As<Value>(); \ | ||
| break; \ | ||
| } \ | ||
| case SQLITE_NULL: { \ | ||
|
|
@@ -2415,6 +2433,11 @@ StatementSync::~StatementSync() { | |
| void StatementSync::Finalize() { | ||
| sqlite3_finalize(statement_); | ||
| statement_ = nullptr; | ||
| InvalidateColumnNameCache(); | ||
| } | ||
|
|
||
| void StatementSync::InvalidateColumnNameCache() { | ||
| cached_column_names_.clear(); | ||
| } | ||
|
|
||
| inline bool StatementSync::IsFinalized() { | ||
|
|
@@ -2598,7 +2621,42 @@ MaybeLocal<Name> StatementSync::ColumnNameToName(const int column) { | |
| return MaybeLocal<Name>(); | ||
| } | ||
|
|
||
| return String::NewFromUtf8(env()->isolate(), col_name).As<Name>(); | ||
| return String::NewFromUtf8( | ||
| env()->isolate(), col_name, NewStringType::kInternalized) | ||
| .As<Name>(); | ||
| } | ||
|
|
||
| // Populates `keys` with cached column names, rebuilding the cache if the | ||
| // statement was re-prepared. | ||
| bool StatementSync::GetCachedColumnNames(LocalVector<Name>* keys) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to this function? |
||
| Isolate* isolate = env()->isolate(); | ||
|
|
||
| const int reprepare_count = | ||
| sqlite3_stmt_status(statement_, SQLITE_STMTSTATUS_REPREPARE, false); | ||
| if (reprepare_count != cached_column_names_reprepare_count_) { | ||
| cached_column_names_.clear(); | ||
| const int num_cols = sqlite3_column_count(statement_); | ||
| if (num_cols == 0) { | ||
| cached_column_names_reprepare_count_ = reprepare_count; | ||
| return true; | ||
| } | ||
| cached_column_names_.reserve(num_cols); | ||
| for (int i = 0; i < num_cols; ++i) { | ||
| Local<Name> key; | ||
| if (!ColumnNameToName(i).ToLocal(&key)) { | ||
| InvalidateColumnNameCache(); | ||
| return false; | ||
| } | ||
| cached_column_names_.emplace_back(Global<Name>(isolate, key)); | ||
| } | ||
| cached_column_names_reprepare_count_ = reprepare_count; | ||
| } | ||
|
|
||
| keys->reserve(cached_column_names_.size()); | ||
| for (const auto& name : cached_column_names_) { | ||
| keys->emplace_back(name.Get(isolate)); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| MaybeLocal<Value> StatementExecutionHelper::ColumnToValue(Environment* env, | ||
|
|
@@ -2620,7 +2678,9 @@ MaybeLocal<Name> StatementExecutionHelper::ColumnNameToName(Environment* env, | |
| return MaybeLocal<Name>(); | ||
| } | ||
|
|
||
| return String::NewFromUtf8(env->isolate(), col_name).As<Name>(); | ||
| return String::NewFromUtf8( | ||
| env->isolate(), col_name, NewStringType::kInternalized) | ||
| .As<Name>(); | ||
| } | ||
|
|
||
| void StatementSync::MemoryInfo(MemoryTracker* tracker) const {} | ||
|
|
@@ -3530,12 +3590,9 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) { | |
| if (iter->stmt_->return_arrays_) { | ||
| row_value = Array::New(isolate, row_values.data(), row_values.size()); | ||
| } else { | ||
| row_keys.reserve(num_cols); | ||
| for (int i = 0; i < num_cols; ++i) { | ||
| Local<Name> key; | ||
| if (!iter->stmt_->ColumnNameToName(i).ToLocal(&key)) return; | ||
| row_keys.emplace_back(key); | ||
| } | ||
| // Use cached internalized column names to avoid repeated V8 string | ||
| // creation and enable hidden class sharing across row objects. | ||
| if (!iter->stmt_->GetCachedColumnNames(&row_keys)) return; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment to here? |
||
|
|
||
| DCHECK_EQ(row_keys.size(), row_values.size()); | ||
| row_value = Object::New( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| #include <optional> | ||
| #include <string_view> | ||
| #include <unordered_set> | ||
| #include <vector> | ||
|
|
||
| namespace node { | ||
| namespace sqlite { | ||
|
|
@@ -277,6 +278,7 @@ class StatementSync : public BaseObject { | |
| static void SetReturnArrays(const v8::FunctionCallbackInfo<v8::Value>& args); | ||
| v8::MaybeLocal<v8::Value> ColumnToValue(const int column); | ||
| v8::MaybeLocal<v8::Name> ColumnNameToName(const int column); | ||
| bool GetCachedColumnNames(v8::LocalVector<v8::Name>* keys); | ||
| void Finalize(); | ||
| bool IsFinalized(); | ||
|
|
||
|
|
@@ -294,6 +296,9 @@ class StatementSync : public BaseObject { | |
| uint64_t reset_generation_ = 0; | ||
| std::optional<std::map<std::string, std::string>> bare_named_params_; | ||
| inline int ResetStatement(); | ||
| std::vector<v8::Global<v8::Name>> cached_column_names_; | ||
| int cached_column_names_reprepare_count_ = -1; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion @louwers but I benchmarked on the iterate path (30 runs) saw a consistent -1% to -2% regression after making these changes: sqlite/sqlite-prepare-select-all.js statement='SELECT text_column FROM foo LIMIT 1' method='iterate' tableSeedSize=100000 n=100000 *** -2.06 % The reason might be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Not worth it then.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it's confirmed, but very interesting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @louwers should we merge this PR? |
||
| void InvalidateColumnNameCache(); | ||
| bool BindParams(const v8::FunctionCallbackInfo<v8::Value>& args); | ||
| bool BindValue(const v8::Local<v8::Value>& value, const int index); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider putting this in a method.