sqlite: optimize column name creation and text value encoding#61954
sqlite: optimize column name creation and text value encoding#61954thisalihassan wants to merge 2 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
thisalihassan
force-pushed
the
sqlite-one-byte-encoding
branch
from
6adb625 to
ff6a788
Compare
Use simdutf to detect ASCII text values and create them via NewFromOneByte for compact one-byte representation. Internalize column name strings with kInternalized so V8 shares hidden classes across row objects. Cache column names on StatementSync for iterate(), invalidated via SQLITE_STMTSTATUS_REPREPARE on schema changes. Refs: nodejs/performance#181
thisalihassan
force-pushed
the
sqlite-one-byte-encoding
branch
from
ff6a788 to
4fcb1ed
Compare
|
I didn't push the changes for |
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61954 +/- ##
==========================================
+ Coverage 88.84% 89.72% +0.88%
==========================================
Files 674 672 -2
Lines 204957 204559 -398
Branches 39309 39274 -35
==========================================
+ Hits 182087 183537 +1450
+ Misses 15088 13326 -1762
+ Partials 7782 7696 -86
π New features to boost your workflow:
|
ronag
requested review from
addaleax,
anonrig and
geeksilva97
and removed request for
geeksilva97
src/node_sqlite.cc
Outdated
| const char* data, | ||
| size_t length) { |
There was a problem hiding this comment.
Can you just take a std::string_view input as an argument rather than this C API like function?
src/node_sqlite.cc
Outdated
| NewStringType::kNormal, | ||
| len); | ||
| } | ||
| return String::NewFromUtf8(isolate, data, NewStringType::kNormal, len); |
There was a problem hiding this comment.
Why don't you use kInternalized here and in line 66?
There was a problem hiding this comment.
Utf8StringMaybeOneByte is used for text cell values (row data), not column names, text values are typically unique user data ("Ali Hassan", "ali.n@example.com", etc.) so the lookup would almost always miss
Column names are already interned separately in ColumnNameToName
| if (!iter->stmt_->ColumnNameToName(i).ToLocal(&key)) return; | ||
| row_keys.emplace_back(key); | ||
| } | ||
| if (!iter->stmt_->GetCachedColumnNames(&row_keys)) return; |
| .As<Name>(); | ||
| } | ||
|
|
||
| bool StatementSync::GetCachedColumnNames(LocalVector<Name>* keys) { |
There was a problem hiding this comment.
Can you add a comment to this function?
thisalihassan
force-pushed
the
sqlite-one-byte-encoding
branch
from
ad603d4 to
fd4215d
Compare
3 participants
Skip the full
UTF-8decode path for text values that are pure ASCII by validating withsimdutfand creatingOneByteV8 strings, halving memory.Internalizecolumn name strings so V8 shares hidden classes across row objects. Cache column names in theiterate()hot loop, invalidating on schema changes viaSQLITE_STMTSTATUS_REPREPARE.Benchmark: 30-run
Refs: nodejs/performance#181