search

nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
240 stars 34 forks source link

Help with node-sqlite3 performance #458

Open daniellockyer opened 11 months ago

daniellockyer commented 11 months ago

Hey all! 👋🏻 I currently maintain node-sqlite3, the most popular set of bindings for sqlite3 with over 800k downloads per week. The Node-API team converted node-sqlite3 from NAN to Node-API in v5, which was released in 2020 (thanks!).

Using Node-API makes the code really nice to understand (especially given I'm not a day-in-day-out C++ dev) and distribute prebuilt binaries for.

The biggest difference between node-sqlite3 and others in the ecosystem is API design (async + callbacks in node-sqlite3 vs sync elsewhere) and performance. I don't doubt async involves overhead, but that's the style that this package is aimed at. If you look at this benchmark, node-sqlite3 is far behind in terms of performance.

I'd like to understand if our use of Node-API could be improved to increase performance, or best practices. The move to Node-API refactored quite a bit of code but we also kept some older concepts around from the NAN implementation.

To try and determine the blockers on performance, I captured this flamegraph from our (somewhat naive) benchmarks:

CleanShot 2024-01-05 at 10 01 51@2x

30% of time is spent in napi_set_named_property, as part of https://github.com/TryGhost/node-sqlite3/blob/ba4ba07f792304c1554f6c5bd70dcb399d0a82d3/src/statement.cc#L790-L823. This code turns the rows from SQLite into an object, so we can push it to an array to return to the user. This would clearly be on the hotpath for a DB fetching benchmark but I can't find any way to improve the performance. I ended up reading https://github.com/nodejs/node/issues/45905, which sounds similar.

17% of the time is spent in napi_create_string_utf8, which seems to have a lot of GC traces beneath it. Similar with napi_create_object. Is this just a red herring because the benchmark creates a lot of new Objects in short succession?

If anyone has any advice for debugging or refactoring, or time to have a poke around, help would be greatly appreciated 🙏🏻

mhdawson commented 10 months ago

I'm not sure if the new API added in https://github.com/nodejs/node/pull/51042/files might help? NAPI_EXPERIMENTAL would need to be enabled.

Will also try to remember to make sure we discuss in the next Node-API team meeting to see if anybody has any suggestions.

mhdawson commented 10 months ago

We discussed a bit in the meeting today, we do think the existing changes in 51042 may help and @legendecas may experiment to see if using it in sqlite speeds things up. It was also mentioned that https://github.com/nodejs/node/issues/45905 is possibly related in terms of issues.

gabrielschulhof commented 8 months ago

@daniellockyer I filed https://github.com/TryGhost/node-sqlite3/pull/1772. It's not a spectacular improvement, but I guess every bit helps.