sqlite: add fast api to DatabaseSync.isTransaction

[anonrig]

This might be a good start to add v8 fast api before we stabilize sqlite. I couldn't find the build error. Maybe @cjihrig might help?

For reproduction, run: make && tools/test.py test/parallel/test-sqlite-database-sync.js

#
# Fatal error in , line 0
# Check failed: v8_flags.fuzzing.
#
#
#
#FailureMessage Object: 0x16ba5da08
----- Native stack trace -----

 1: 0x1044fd394 node::NodePlatform::GetStackTracePrinter()::$_0::__invoke() [/Users/yagiz/coding/node/out/Release/node]
 2: 0x105b63c28 V8_Fatal(char const*, ...) [/Users/yagiz/coding/node/out/Release/node]
 3: 0x106288ab0 v8::internal::Runtime_OptimizeOsr(int, unsigned long*, v8::internal::Isolate*) (.cold.1) [/Users/yagiz/coding/node/out/Release/node]
 4: 0x104c5ced8 v8::internal::Runtime_OptimizeOsr(int, unsigned long*, v8::internal::Isolate*) [/Users/yagiz/coding/node/out/Release/node]
 5: 0x105258d08 Builtins_CEntry_Return1_ArgvInRegister_NoBuiltinExit [/Users/yagiz/coding/node/out/Release/node]
 6: 0x10533f9b4 Builtins_CallRuntimeHandler [/Users/yagiz/coding/node/out/Release/node]
 7: 0x1051bc9f8 Builtins_InterpreterEntryTrampoline [/Users/yagiz/coding/node/out/Release/node]     8: 0x1051bc9f8 Builtins_InterpreterEntryTrampoline [/Users/yagiz/coding/node/out/Release/node]     9: 0x1136e5638                                                                                    10: 0x1136f88c8                                                                                    11: 0x1136e96d8                                                                                    12: 0x1136f5afc
13: 0x1136f9bec
14: 0x1051fb8c8 Builtins_AsyncFunctionAwaitResolveClosure [/Users/yagiz/coding/node/out/Release/node]                                                                                                 15: 0x1052ce0d8 Builtins_PromiseFulfillReactionJob [/Users/yagiz/coding/node/out/Release/node]
16: 0x1051eacd0 Builtins_RunMicrotasks [/Users/yagiz/coding/node/out/Release/node]                 17: 0x1051ba550 Builtins_JSRunMicrotasksEntry [/Users/yagiz/coding/node/out/Release/node]          18: 0x10478ccb0 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/yagiz/coding/node/out/Release/node]
19: 0x10478d574 v8::internal::(anonymous namespace)::InvokeWithTryCatch(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/yagiz/coding/node/out/Release/node] 20: 0x10478d694 v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) [/Users/yagiz/coding/node/out/Release/node]
21: 0x106264148 v8::internal::MicrotaskQueue::RunMicrotasks(v8::internal::Isolate*) [/Users/yagiz/coding/node/out/Release/node]
22: 0x106264438 v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) (.cold.1) [/Users/yagiz/coding/node/out/Release/node]
23: 0x1047b9e08 v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) [/Users/yagiz/coding/node/out/Release/node]
24: 0x1051beb38 Builtins_CallApiCallbackGeneric [/Users/yagiz/coding/node/out/Release/node]        25: 0x1051bc9f8 Builtins_InterpreterEntryTrampoline [/Users/yagiz/coding/node/out/Release/node]
26: 0x1051ba66c Builtins_JSEntryTrampoline [/Users/yagiz/coding/node/out/Release/node]             27: 0x1051ba310 Builtins_JSEntry [/Users/yagiz/coding/node/out/Release/node]                       28: 0x10478ccf0 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/yagiz/coding/node/out/Release/node]             29: 0x10478c680 v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/Users/yagiz/coding/node/out/Release/node]                                              30: 0x104638bac v8::Function::Call(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/Users/yagiz/coding/node/out/Release/node]                                31: 0x1043c5e78 node::InternalCallbackScope::Close() [/Users/yagiz/coding/node/out/Release/node]
32: 0x1043c588c node::InternalCallbackScope::~InternalCallbackScope() [/Users/yagiz/coding/node/out/Release/node]                                                                                     33: 0x10445ba88 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) [/Users/yagiz/coding/node/out/Release/node]
34: 0x1043ca828 node::LoadEnvironment(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>, std::__1::function<void (node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Value>)>) [/Users/yagiz/coding/node/out/Release/node]                35: 0x1044c9c28 node::NodeMainInstance::Run() [/Users/yagiz/coding/node/out/Release/node]          36: 0x10445f7ac node::Start(int, char**) [/Users/yagiz/coding/node/out/Release/node]
37: 0x18fdec274 start [/usr/lib/dyld]                                                              Command: out/Release/node --expose-internals --allow-natives-syntax --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/yagiz/coding/node/test/parallel/test-sqlite-database-sync.js
--- CRASHED (Signal: 5) ---

                                                                                                   [00:00|% 100|+   0|-   1]: Done
Failed tests:
out/Release/node --expose-internals --allow-natives-syntax --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/yagiz/coding/node/test/parallel/test-sqlite-database-sync.js

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (4cd8e19) to head (5a9ad71).

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	68.42%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57952      +/-   ##
==========================================
- Coverage   90.27%   90.27%   -0.01%     
==========================================
  Files         630      630              
  Lines      186328   186343      +15     
  Branches    36503    36503              
==========================================
+ Hits       168205   168217      +12     
- Misses      11001    11009       +8     
+ Partials     7122     7117       -5     

Files with missing lines	Coverage Δ
src/node_sqlite.h	68.00% <ø> (ø)
src/node_sqlite.cc	80.38% <68.42%> (-0.18%)	⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cjihrig]

The crash happens in the test on this line:

eval('%PrepareFunctionForOptimization(db.isTransaction)');

Changing the line to DatabaseSync.prototype.isTransaction does not work either. Do we know if getters are supported by the fast API?

[anonrig]

Do we know if getters are supported by the fast API?

@joyeecheung @jasnell maybe you know?

[jasnell]

I do not know, sorry.

[H4ad]

Maybe you can try expose it via getter on https://github.com/nodejs/node/blob/02849f83978debf8fb66453bfbae0ab7ae2cb07a/lib/sqlite.js, and then add the fastAPI in a method exposed by the sqlite binding.

[anonrig]

Maybe you can try expose it via getter on https://github.com/nodejs/node/blob/02849f83978debf8fb66453bfbae0ab7ae2cb07a/lib/sqlite.js, and then add the fastAPI in a method exposed by the sqlite binding.

@cjihrig @H4ad With the help of @erikcorry we found the issue, it seems the issue is with optimizing functions through OptimizeFunctionOnNextCall. We can not optimize a C++ function, we have to surround it with a JavaScript function first.

[cjihrig]

Interesting. That seems like something worth documenting in https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md.

[anonrig]

Interesting. That seems like something worth documenting in https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md.

I'll open a PR but first I need to wait for a response at the v8-dev - https://groups.google.com/g/v8-dev/c/Mr-1Dx7clBY

[H4ad]

Side note: Do we have benchmarks for sqlite (at least I didn't found)? I don't doubt this will help, but would be great to have something to compare and see the improvements

[anonrig]

Side note: Do we have benchmarks for sqlite (at least I didn't found)? I don't doubt this will help, but would be great to have something to compare and see the improvements

I don't think there is, but we should definitely have. It seems the tests are not passing because fast path counter isn't increased. I wonder if the fast path isn't invoked because we're testing it on debug builds? Can it be possible @targos @joyeecheung?

[H4ad]

I don't think there is, but we should definitely have.

I will open a PR to add some benchmarks