From: stephan Date: Sun, 23 Nov 2025 11:28:27 +0000 (+0000) Subject: Replace an alloc/free on every read of a JS kvvfs key with a one-time alloc and a... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48c8405f99ebc636da4c4d5bff76f16cdff9fea9;p=thirdparty%2Fsqlite.git Replace an alloc/free on every read of a JS kvvfs key with a one-time alloc and a long paragraph explaining why we have to leak it. FossilOrigin-Name: 66fb9978f0a63887d214e895343adedfa46ee7aefccb8389d6d3af727e0a65ea --- diff --git a/ext/wasm/api/sqlite3-api-prologue.js b/ext/wasm/api/sqlite3-api-prologue.js index 13ef3f1c67..94112a86da 100644 --- a/ext/wasm/api/sqlite3-api-prologue.js +++ b/ext/wasm/api/sqlite3-api-prologue.js @@ -892,7 +892,7 @@ globalThis.sqlite3ApiBootstrap = async function sqlite3ApiBootstrap( Like this.alloc.impl(), this.realloc.impl() is a direct binding to the underlying realloc() implementation which does not throw - exceptions, instead returning 0 on allocation error. + exceptions, instead returning 0 (or 0n) on allocation error. */ realloc: undefined/*installed later*/, diff --git a/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js b/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js index e1ed474ff4..1d19cacc79 100644 --- a/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js +++ b/ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js @@ -308,18 +308,16 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ const pVfs = new capi.sqlite3_vfs(kvvfsMethods.$pVfs); const pIoDb = new capi.sqlite3_io_methods(kvvfsMethods.$pIoDb); const pIoJrnl = new capi.sqlite3_io_methods(kvvfsMethods.$pIoJrnl); - const recordHandler = Object.create(null); + const recordHandler = + Object.create(null)/** helper for some vfs + routines. Populated later. */; /** Implementations for members of the object referred to by - sqlite3__wasm_kvvfs_methods(). We swap out the native + sqlite3__wasm_kvvfs_methods(). We swap out some native implementations with these, which use JS Storage for their backing store. - - The interface docs for these methods are in - src/os_kv.c:kvrecordRead(), kvrecordWrite(), and - kvrecordDelete(). */ - sqlite3_kvvfs_methods.override = { + const methodOverrides = { /** sqlite3_kvvfs_methods's member methods. These perform the @@ -327,11 +325,21 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ kvvfs. In the native impl these write each db page to a separate file. This impl stores each db page as a single record in a Storage object which is mapped to zClass. + + A db's size is stored in a record named kvvfs[-storagename]-sz + and the journal is stored in kvvfs[-storagename]-jrnl. The + [-storagename] part is a remnant of the native impl (so that + it has unique filenames per db) and is only used for + localStorage and sessionStorage. We elide that part (to save + space) from other storage objects but retain it on those two + to avoid invalidating pre-version-2 session/localStorage dbs. + + The interface docs for these methods are in src/os_kv.c's + kvrecordRead(), kvrecordWrite(), and kvrecordDelete(). */ recordHandler: { xRcrdRead: (zClass, zKey, zBuf, nBuf)=>{ - const stack = pstack.pointer, - astack = wasm.scopedAllocPush(); + const stack = pstack.pointer /* keyForStorage() allocs from here */; try{ const store = storageForZClass(zClass); if( 0 ){ @@ -354,31 +362,41 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ wasm.poke(zBuf, 0); return nV; } + util.assert( nBuf>nV, "xRcrdRead Input buffer is too small" ); if( 0 ){ debug("xRcrdRead", nBuf, zClass, wasm.cstrToJs(zClass), wasm.cstrToJs(zKey), nV, jV, store); } - const zV = wasm.scopedAllocCString(jV) - /* TODO: allocate a single 128kb buffer (largest page - size) for reuse here, or maybe even preallocate - it somewhere in sqlite3__wasm_kvvfs_...(). */; - if(nBuf > nV + 1) nBuf = nV + 1; - wasm.heap8u().copyWithin( - Number(zBuf), Number(zV), wasm.ptr.addn(zV, nBuf,- 1) + const zV = cache.keybuf.mem ??= wasm.alloc.impl(cache.keybuf.n) + /* We leak this one-time alloc because we've no better + option. sqlite3_vfs does not have a finalizer, so + we've no place to hook in the cleanup. We "could" + extend sqlite3_shutdown() to have a cleanup list for + stuff like this but (A) that function is never used in + JS and (B) its cleanup would leave cache.keybuf + pointing to stale memory, so if the library were used + after sqlite3_shutdown() then we'd corrupt memory. */; + if( !zV ) return -3 /*OOM*/; + const heap = wasm.heap8(); + let i; + for(i = 0; i < nV; ++i){ + heap[wasm.ptr.add(zV,i)] = jV.codePointAt(i) & 0xFF; + } + heap.copyWithin( + Number(zBuf), Number(zV), wasm.ptr.addn(zV, i) ); - wasm.poke(wasm.ptr.add(zBuf, nBuf), 0); + heap[wasm.ptr.add(zBuf, nV)] = 0; return nBuf; }catch(e){ error("kvrecordRead()",e); return -2; }finally{ pstack.restore(stack); - wasm.scopedAllocPop(astack); } }, xRcrdWrite: (zClass, zKey, zData)=>{ - const stack = pstack.pointer; + const stack = pstack.pointer /* keyForStorage() allocs from here */; try { const store = storageForZClass(zClass); const zXKey = keyForStorage(store, zClass, zKey); @@ -397,7 +415,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ }, xRcrdDelete: (zClass, zKey)=>{ - const stack = pstack.pointer; + const stack = pstack.pointer /* keyForStorage() allocs from here */; try { const store = storageForZClass(zClass); const zXKey = keyForStorage(store, zClass, zKey); @@ -415,8 +433,8 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ /** Override certain operations of the underlying sqlite3_vfs and - two sqlite3_io_methods instances so that we can tie Storage - objects to db names. + the two sqlite3_io_methods instances so that we can tie + Storage objects to db names. */ vfs:{ /* sqlite3_kvvfs_methods::pVfs's methods */ @@ -427,44 +445,42 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ if( n > kvvfsMethods.$nKeySize - 8 /*"-journal"*/ - 1 ){ warn("file name is too long:", wasm.cstrToJs(zName)); return capi.SQLITE_RANGE; - }else{ - wasm.poke32(pOutFlags, flags | sqlite3.SQLITE_OPEN_CREATE); } + //wasm.poke32(pOutFlags, flags | sqlite3.SQLITE_OPEN_CREATE); const rc = originalMethods.vfs.xOpen(pProtoVfs, zName, pProtoFile, flags, pOutFlags); - if( 0==rc ){ - const f = new KVVfsFile(pProtoFile); - util.assert(f.$zClass, "Missing f.$zClass"); - const jzClass = wasm.cstrToJs(zName);//f.$zClass); - let s = cache.jzClassToStorage[jzClass]; - //debug("xOpen", jzClass, s); - if( s ){ - ++s.refc; - }else{ - /* TODO: a url flag which tells it to keep the storage - around forever so that future xOpen()s get the same - Storage-ish objects. We can accomplish that by - simply increasing the refcount once more. */ - util.assert( !f.$isJournal, "Opening a journal before its db? "+jzClass ); - const other = f.$isJournal - ? jzClass.replace(cache.rxJournalSuffix,'') - : jzClass + '-journal'; - s = cache.jzClassToStorage[jzClass] - = cache.jzClassToStorage[other] - = Object.assign(Object.create(null),{ - jzClass: jzClass, - refc: 1/* if this is a db-open, the journal open - will follow soon enough and bump the - refcount. If we start at 2 here, that - pending open will increment it again. */, - s: new TransientStorage - }); - debug("xOpen installed storage handles [", - jzClass, other,"]", s); - } - pFileHandles.set(pProtoFile, {s,f,n:jzClass}); + if( rc ) return rc; + const f = new KVVfsFile(pProtoFile); + util.assert(f.$zClass, "Missing f.$zClass"); + const jzClass = wasm.cstrToJs(zName);//f.$zClass); + let s = cache.jzClassToStorage[jzClass]; + //debug("xOpen", jzClass, s); + if( s ){ + ++s.refc; + }else{ + /* TODO: a url flag which tells it to keep the storage + around forever so that future xOpen()s get the same + Storage-ish objects. We can accomplish that by + simply increasing the refcount once more. */ + util.assert( !f.$isJournal, "Opening a journal before its db? "+jzClass ); + const other = f.$isJournal + ? jzClass.replace(cache.rxJournalSuffix,'') + : jzClass + '-journal'; + s = cache.jzClassToStorage[jzClass] + = cache.jzClassToStorage[other] + = Object.assign(Object.create(null),{ + jzClass: jzClass, + refc: 1/* if this is a db-open, the journal open + will follow soon enough and bump the + refcount. If we start at 2 here, that + pending open will increment it again. */, + s: new TransientStorage + }); + debug("xOpen installed storage handles [", + jzClass, other,"]", s); } - return rc; + pFileHandles.set(pProtoFile, {s,f,n:jzClass}); + return 0; }catch(e){ warn("xOpen:",e); return capi.SQLITE_ERROR; @@ -500,7 +516,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ mystery. */ wasm.poke32(pResOut, 0); - return 0; + if( 1 ) return 0; if( 1 ){ /* This is fundamentally exactly what the native impl does. */ const jzName = wasm.cstrToJs(zPath); @@ -628,20 +644,23 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ xDeviceCharacteristics: true //#endif } - }/*sqlite3_kvvfs_methods.override*/; + }/*methodOverrides*/; - const ov = sqlite3_kvvfs_methods.override; debug("pVfs and friends", pVfs, pIoDb, pIoJrnl, kvvfsMethods, capi.sqlite3_file.structInfo, KVVfsFile.structInfo); try { - for(const e of Object.entries(ov.recordHandler)){ + cache.keybuf = Object.create(null); + cache.keybuf.n = kvvfsMethods.$nBufferSize; + util.assert( cache.keybuf.n>1024*129, "Key buffer is not large enough" + /* Native is SQLITE_KVOS_SZ is 133073 as of this writing */ ); + for(const e of Object.entries(methodOverrides.recordHandler)){ // Overwrite kvvfsMethods's callbacks recordHandler[e[0]] = e[1]; kvvfsMethods[kvvfsMethods.memberKey(e[0])] = wasm.installFunction(kvvfsMethods.memberSignature(e[0]), e[1]); } - for(const e of Object.entries(ov.vfs)){ + for(const e of Object.entries(methodOverrides.vfs)){ // Overwrite some pVfs entries and stash the original impls const k = e[0], f = e[1], km = pVfs.memberKey(k), member = pVfs.structInfo.members[k] @@ -650,14 +669,14 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ || util.toss("Missing native pVfs[",km,"]"); pVfs[km] = wasm.installFunction(member.signature, f); } - for(const e of Object.entries(ov.ioDb)){ + for(const e of Object.entries(methodOverrides.ioDb)){ // Similar treatment for pVfs.$pIoDb a.k.a. pIoDb... const k = e[0], f = e[1], km = pIoDb.memberKey(k); originalMethods.ioDb[k] = wasm.functionEntry(pIoDb[km]) || util.toss("Missing native pIoDb[",km,"]"); pIoDb[km] = wasm.installFunction(pIoDb.memberSignature(k), f); } - for(const e of Object.entries(ov.ioJrnl)){ + for(const e of Object.entries(methodOverrides.ioJrnl)){ // Similar treatment for pVfs.$pIoJrnl a.k.a. pIoJrnl... const k = e[0], f = e[1], km = pIoJrnl.memberKey(k); originalMethods.ioJrnl[k] = wasm.functionEntry(pIoJrnl[km]) @@ -675,7 +694,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ pIoDb.dispose(); pIoJrnl.dispose(); } - }/*method overrides*/ + }/*native method overrides*/ if(sqlite3?.oo1?.DB){ /** diff --git a/manifest b/manifest index 80cae3eacd..68253d1172 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C This\scombination\sof\skvvfs\scallbacks\sseems\sto\swork\sokay\sfor\sboth\spersistent\sand\stransient\sstorage,\sbut\sit's\sunclear\swhy/how,\sas\sxAccess()\sdoes\snot\sappear\sto\sbe\sdoing\sanything\suseful\s(and\smisbehaves\swhen\stold\sto\sreport\sthat\sthe\sstorage\sobject\sin\squestion\sexists).\sThese\stests\sfail\sin\s64-bit\sbuilds\sfor\sas-yet-unknown\sreasons\s(other\stests\scontinue\sto\spass\sin\s64-bit). -D 2025-11-22T19:21:25.670 +C Replace\san\salloc/free\son\severy\sread\sof\sa\sJS\skvvfs\skey\swith\sa\sone-time\salloc\sand\sa\slong\sparagraph\sexplaining\swhy\swe\shave\sto\sleak\sit. +D 2025-11-23T11:28:27.602 F .fossil-settings/binary-glob 61195414528fb3ea9693577e1980230d78a1f8b0a54c78cf1b9b24d0a409ed6a x F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea @@ -595,12 +595,12 @@ F ext/wasm/api/post-js-header.js d24bd0d065f3489c8b78ddf3ead6321e5d047187a162cd5 F ext/wasm/api/pre-js.c-pp.js ad2546290e0c8ce5ca2081bff6e85cc25eeb904a3303921f1184290a7ff1b32f F ext/wasm/api/sqlite3-api-glue.c-pp.js 9eaed1801be392f6687aa7da8e3a5a41d03de19993d8fe62ee6c52617eab4985 F ext/wasm/api/sqlite3-api-oo1.c-pp.js 7d8850f754b4a9aecd5a4a92a357e05f720cd7f5cf962909343b40c914237256 -F ext/wasm/api/sqlite3-api-prologue.js 29bc41d0f9fbca946fc60681fb79901b3224988b5b978a47620595bbe0e4cf84 +F ext/wasm/api/sqlite3-api-prologue.js 2ac62b41dd8d66859c86a6af126690851e5e557dad61ef59692389762c9bd2ed F ext/wasm/api/sqlite3-api-worker1.c-pp.js 1041dd645e8e821c082b628cd8d9acf70c667430f9d45167569633ffc7567938 F ext/wasm/api/sqlite3-license-version-header.js 0c807a421f0187e778dc1078f10d2994b915123c1223fe752b60afdcd1263f89 F ext/wasm/api/sqlite3-opfs-async-proxy.js 9654b565b346dc609b75d15337f20acfa7af7d9d558da1afeb9b6d8eaa404966 F ext/wasm/api/sqlite3-vfs-helper.c-pp.js 3f828cc66758acb40e9c5b4dcfd87fd478a14c8fb7f0630264e6c7fa0e57515d -F ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js bd461cf2e1c35aab71ce43906b514d9f5bb348cbbcadf0bac46d53a3ab84af66 +F ext/wasm/api/sqlite3-vfs-kvvfs.c-pp.js 6a7007c26361b9d9b74d0740c61eea5d93253675824422b100be38b9cbe25073 F ext/wasm/api/sqlite3-vfs-opfs-sahpool.c-pp.js 26cb41d5a62f46a106b6371eb00fef02de3cdbfaa51338ba087a45f53028e0d0 F ext/wasm/api/sqlite3-vfs-opfs.c-pp.js 88ce2078267a2d1af57525a32d896295f4a8db7664de0e17e82dc9ff006ed8d3 F ext/wasm/api/sqlite3-vtab-helper.c-pp.js 9097074724172e31e56ce20ccd7482259cf72a76124213cbc9469d757676da86 @@ -2178,8 +2178,8 @@ F tool/version-info.c 33d0390ef484b3b1cb685d59362be891ea162123cea181cb8e6d2cf6dd F tool/warnings-clang.sh bbf6a1e685e534c92ec2bfba5b1745f34fb6f0bc2a362850723a9ee87c1b31a7 F tool/warnings.sh d924598cf2f55a4ecbc2aeb055c10bd5f48114793e7ba25f9585435da29e7e98 F tool/win/sqlite.vsix deb315d026cc8400325c5863eef847784a219a2f -P 0b53be562f1e1a5b20ffe8d72df64e753a8d759b580d949a0f32409144769bb0 -R 18ff16459471e0fa48753f413430772f +P 69e591d0054218ead789cee199e5258f1c378a89e4b7b0e38fe74e834e23156b +R 59e8541a0d2f7929ff70c46115b7160f U stephan -Z 100d31fa602c6a076823477a2cb4e392 +Z dc6a511f30ab2f711394f8b6dd32e86b # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 0b625bdcad..194b2617d1 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -69e591d0054218ead789cee199e5258f1c378a89e4b7b0e38fe74e834e23156b +66fb9978f0a63887d214e895343adedfa46ee7aefccb8389d6d3af727e0a65ea