Skip to content

Commit

Permalink
SQLite: Refactor: Have Cursor::iteratorImpl() convert all the way to …
Browse files Browse the repository at this point in the history 
…JsValue.

This is much cleaner as `SqlValue` may contain pointers that are invalidated when we iterate the statement. Previously, we were counting on such iteration not happening before JSG had a chance to convert the values to JavaScript, which was a little precarious.

This will also allow us -- in a later commit -- to implement an optimization where we iterate the statement more eagerly.

With this change, the logic of `wrapSqlRow{,Raw}()` ends up being merged into `{row,raw}IteratorNext()` so the former can be deleted.
  • Loading branch information
@kentonv
kentonv committed Oct 29, 2024
1 parent e9df309 commit 1da3c7e
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 65 deletions.
74 changes: 27 additions & 47 deletions src/workerd/api/sql.c++
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,26 +131,6 @@ jsg::JsValue SqlStorage::wrapSqlValue(jsg::Lock& js, SqlValue value) {
}
}

jsg::JsObject SqlStorage::wrapSqlRow(jsg::Lock& js, SqlRow row) {
return jsg::JsObject(js.withinHandleScope([&]() -> v8::Local<v8::Object> {
jsg::JsObject result = js.obj();
for (auto& field: row.fields) {
result.set(js, field.name, wrapSqlValue(js, kj::mv(field.value)));
}
return result;
}));
}

jsg::JsArray SqlStorage::wrapSqlRowRaw(jsg::Lock& js, kj::Array<SqlValue> row) {
return jsg::JsArray(js.withinHandleScope([&]() -> v8::Local<v8::Array> {
v8::LocalVector<v8::Value> values(js.v8Isolate);
for (auto& field: row) {
values.push_back(wrapSqlValue(js, kj::mv(field)));
}
return v8::Array::New(js.v8Isolate, values.data(), values.size());
}));
}

SqlStorage::Cursor::State::State(SqliteDatabase& db,
SqliteDatabase::Regulator& regulator,
kj::StringPtr sqlCode,
Expand Down Expand Up @@ -228,7 +208,7 @@ jsg::JsArray SqlStorage::Cursor::toArray(jsg::Lock& js) {
for (;;) {
auto maybeRow = rowIteratorNext(js, self);
KJ_IF_SOME(row, maybeRow) {
results.push_back(wrapSqlRow(js, kj::mv(row)));
results.push_back(row);
} else {
break;
}
Expand All @@ -243,9 +223,8 @@ jsg::JsValue SqlStorage::Cursor::one(jsg::Lock& js) {
}

auto self = JSG_THIS;
auto row = JSG_REQUIRE_NONNULL(rowIteratorNext(js, self), Error,
auto result = JSG_REQUIRE_NONNULL(rowIteratorNext(js, self), Error,
"Expected exactly one result from SQL query, but got no results.");
auto result = wrapSqlRow(js, kj::mv(row));

// Manually grab the query, check that there are no more results, and clean it up.
auto& query = KJ_ASSERT_NONNULL(state)->query;
Expand All @@ -272,17 +251,18 @@ jsg::Ref<SqlStorage::Cursor::RowIterator> SqlStorage::Cursor::rows(jsg::Lock& js
return jsg::alloc<RowIterator>(JSG_THIS);
}

kj::Maybe<SqlStorage::SqlRow> SqlStorage::Cursor::rowIteratorNext(
jsg::Lock& js, jsg::Ref<Cursor>& obj) {
kj::Maybe<jsg::JsObject> SqlStorage::Cursor::rowIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj) {
auto names = obj->cachedColumnNames.get();
return iteratorImpl(js, obj, [&](State& state, uint i, SqlValue&& value) {
return SqlRow::Field{
// A little trick here: We know there are no HandleScopes on the stack between JSG and here,
// so we can return a dict keyed by local handles, which avoids constructing new V8Refs here
// which would be relatively slower.
.name = names[i].getHandle(js),
.value = kj::mv(value)};
}).map([&](kj::Array<SqlRow::Field>&& fields) { return SqlRow{.fields = kj::mv(fields)}; });
KJ_IF_SOME(values, iteratorImpl(js, obj)) {
jsg::JsObject result = js.obj();
KJ_ASSERT(names.size() == values.size());
for (auto i: kj::zeroTo(names.size())) {
result.set(js, names[i].getHandle(js), jsg::JsValue(values[i]));
}
return result;
} else {
return kj::none;
}
}

jsg::Ref<SqlStorage::Cursor::RawIterator> SqlStorage::Cursor::raw(jsg::Lock&) {
Expand All @@ -301,18 +281,16 @@ kj::Array<jsg::JsRef<jsg::JsString>> SqlStorage::Cursor::getColumnNames(jsg::Loc
}
}

kj::Maybe<kj::Array<SqlStorage::SqlValue>> SqlStorage::Cursor::rawIteratorNext(
jsg::Lock& js, jsg::Ref<Cursor>& obj) {
return iteratorImpl(
js, obj, [&](State& state, uint i, SqlValue&& value) { return kj::mv(value); });
kj::Maybe<jsg::JsArray> SqlStorage::Cursor::rawIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj) {
KJ_IF_SOME(values, iteratorImpl(js, obj)) {
return jsg::JsArray(v8::Array::New(js.v8Isolate, values.data(), values.size()));
} else {
return kj::none;
}
}

template <typename Func>
auto SqlStorage::Cursor::iteratorImpl(jsg::Lock& js, jsg::Ref<Cursor>& obj, Func&& func)
-> kj::Maybe<
kj::Array<decltype(func(kj::instance<State&>(), uint(), kj::instance<SqlValue&&>()))>> {
using Element = decltype(func(kj::instance<State&>(), uint(), kj::instance<SqlValue&&>()));

kj::Maybe<v8::LocalVector<v8::Value>> SqlStorage::Cursor::iteratorImpl(
jsg::Lock& js, jsg::Ref<Cursor>& obj) {
auto& state = *KJ_UNWRAP_OR(obj->state, {
if (obj->canceled) {
JSG_FAIL_REQUIRE(Error,
Expand Down Expand Up @@ -344,8 +322,10 @@ auto SqlStorage::Cursor::iteratorImpl(jsg::Lock& js, jsg::Ref<Cursor>& obj, Func
return kj::none;
}

auto results = kj::heapArrayBuilder<Element>(query.columnCount());
for (auto i: kj::zeroTo(results.capacity())) {
auto n = query.columnCount();
v8::LocalVector<v8::Value> results(js.v8Isolate);
results.reserve(n);
for (auto i: kj::zeroTo(n)) {
SqlValue value;
KJ_SWITCH_ONEOF(query.getValue(i)) {
KJ_CASE_ONEOF(data, kj::ArrayPtr<const byte>) {
Expand All @@ -367,9 +347,9 @@ auto SqlStorage::Cursor::iteratorImpl(jsg::Lock& js, jsg::Ref<Cursor>& obj, Func
// leave value null
}
}
results.add(func(state, i, kj::mv(value)));
results.push_back(wrapSqlValue(js, kj::mv(value)));
}
return results.finish();
return kj::mv(results);
}

kj::Array<const SqliteDatabase::Query::ValuePtr> SqlStorage::Cursor::mapBindings(
Expand Down
29 changes: 11 additions & 18 deletions src/workerd/api/sql.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
// JSG, which does not need to make a copy.
using SqlValue = kj::Maybe<kj::OneOf<kj::Array<byte>, kj::StringPtr, double>>;

// One row of a SQL query result. This is an Object whose properties correspond to columns.
using SqlRow = jsg::Dict<SqlValue, jsg::JsString>;

jsg::Ref<Cursor> exec(jsg::Lock& js, jsg::JsString query, jsg::Arguments<BindingValue> bindings);
IngestResult ingest(jsg::Lock& js, kj::String query);

Expand Down Expand Up @@ -186,18 +183,17 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
}
}

// Utility functions to convert SqlValue, SqlRow, and Array<SqlValue> to JS values. In some
// cases we end up having to do this conversion before actually returning, so we can't have
// JSG do it. We can't use jsg::TypeHandler because SqlValue contains StringPtr, which doesn't
// support unwrapping. We don't actually ever use unwrapping, but requesting a TypeHandler forces
// JSG to try to generate the code for unwrapping, leading to compiler errors.
// Utility functions to convert SqlValue to a JS value. We can't just return the C++ values and
// let JSG do the work because we're trying to avoid having to make a copy of string contents out
// of SQLite's buffer when the conversion to JS is just going to make another copy. We can't use
// jsg::TypeHandler because SqlValue contains StringPtr, which doesn't support unwrapping. We
// don't actually ever use unwrapping, but requesting a TypeHandler forces JSG to try to generate
// the code for unwrapping, leading to compiler errors.
//
// TODO(cleanup): Think hard about how to make JSG support this better. Part of the problem is
// that we're being too clever with optimizations to avoid copying strings when we don't need
// to.
static jsg::JsValue wrapSqlValue(jsg::Lock& js, SqlValue value);
static jsg::JsObject wrapSqlRow(jsg::Lock& js, SqlRow row);
static jsg::JsArray wrapSqlRowRaw(jsg::Lock& js, kj::Array<SqlValue> row);
};

class SqlStorage::Cursor final: public jsg::Object {
Expand Down Expand Up @@ -234,8 +230,8 @@ class SqlStorage::Cursor final: public jsg::Object {
});
}

JSG_ITERATOR(RowIterator, rows, SqlRow, jsg::Ref<Cursor>, rowIteratorNext);
JSG_ITERATOR(RawIterator, raw, kj::Array<SqlValue>, jsg::Ref<Cursor>, rawIteratorNext);
JSG_ITERATOR(RowIterator, rows, jsg::JsObject, jsg::Ref<Cursor>, rowIteratorNext);
JSG_ITERATOR(RawIterator, raw, jsg::JsArray, jsg::Ref<Cursor>, rawIteratorNext);

RowIterator::Next next(jsg::Lock& js);
jsg::JsArray toArray(jsg::Lock& js);
Expand Down Expand Up @@ -293,12 +289,9 @@ class SqlStorage::Cursor final: public jsg::Object {
static kj::Array<const SqliteDatabase::Query::ValuePtr> mapBindings(
kj::ArrayPtr<BindingValue> values);

static kj::Maybe<SqlRow> rowIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj);
static kj::Maybe<kj::Array<SqlValue>> rawIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj);
template <typename Func>
static auto iteratorImpl(jsg::Lock& js, jsg::Ref<Cursor>& obj, Func&& func)
-> kj::Maybe<
kj::Array<decltype(func(kj::instance<State&>(), uint(), kj::instance<SqlValue&&>()))>>;
static kj::Maybe<jsg::JsObject> rowIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj);
static kj::Maybe<jsg::JsArray> rawIteratorNext(jsg::Lock& js, jsg::Ref<Cursor>& obj);
static kj::Maybe<v8::LocalVector<v8::Value>> iteratorImpl(jsg::Lock& js, jsg::Ref<Cursor>& obj);

friend class Statement;
};
Expand Down

0 comments on commit 1da3c7e

Please sign in to comment.