Reuse cached prepared statements in ECSql synchronous reader path#9542
Open
khanaffan wants to merge 3 commits into
Open
Reuse cached prepared statements in ECSql synchronous reader path#9542khanaffan wants to merge 3 commits into
khanaffan wants to merge 3 commits into
Conversation
ECSqlRowExecutor now retrieves a prepared statement from the owning db's statement cache (via the new internal [_getStatementCache] accessor on ECDb and IModelDb) instead of re-preparing the ECSQL on every call in the synchronous withQueryReader path. This fixes a per-element performance regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Restores prepared ECSQL statement reuse for the synchronous withQueryReader execution path by routing ECSqlRowExecutor through the existing per-db StatementCache, addressing a per-element prepare/compile performance regression.
Changes:
- Added an internal
[_getStatementCache]()symbol accessor onIModelDbandECDbto expose the existing ECSQLStatementCacheto internal collaborators. - Updated
ECSqlRowExecutorto checkout prepared statements from the cache (or prepare once) and return them to the cache on disposal. - Added regression tests validating reuse, cache round-trip, nested-reader independence, and prepare-failure recovery for both
IModelDbandECDbreaders.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/backend/src/test/ecdb/ECSqlSyncReader.test.ts | Adds regression coverage for prepared-statement reuse and cache behavior in sync readers. |
| core/backend/src/internal/Symbols.ts | Introduces _getStatementCache internal symbol for cache access. |
| core/backend/src/IModelDb.ts | Exposes ECSQL statement cache via [_getStatementCache]() for internal use. |
| core/backend/src/ECSqlRowExecutor.ts | Reuses cached prepared statements and returns them to the cache on dispose. |
| core/backend/src/ECDb.ts | Exposes ECSQL statement cache via [_getStatementCache]() for internal use. |
| common/changes/@itwin/core-backend/statement-cache-withqueryreader.json | Records the change (type none) for lockstep release tracking. |
| common/api/core-backend.api.md | Updates API report with new @internal [_getStatementCache]() entries. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
core/backend/src/test/ecdb/ECSqlSyncReader.test.ts:165
- These assertions reach into the private
_statementCacheviaany. With_getStatementCacheavailable, the test can verify cache behavior through the intended internal accessor and avoid depending on the private field name.
expect((iModel as any)._statementCache.size).to.equal(0);
iModel.withQueryReader(sql, (reader) => reader.step(), new QueryBinder().bindId(1, "0x1"));
expect((iModel as any)._statementCache.size).to.be.greaterThan(0);
// A subsequent call with the same SQL must be able to check the cached statement back out.
const found = (iModel as any)._statementCache.findAndRemove(sql);
core/backend/src/test/ecdb/ECSqlSyncReader.test.ts:192
- Same here: prefer using the new internal
_getStatementCacheaccessor over(iModel as any)._statementCacheso the test doesn't depend on a private field name.
expect((iModel as any)._statementCache.findAndRemove("SELECT * FROM bis.ThisClassDoesNotExist")).to.be.undefined;
core/backend/src/test/ecdb/ECSqlSyncReader.test.ts:212
- Prefer using the new internal
_getStatementCacheaccessor over(ecdb as any)._statementCacheto avoidanyand reliance on a private field name.
expect((ecdb as any)._statementCache.size).to.be.greaterThan(0);
Comment on lines
+12
to
15
| import { ECSqlStatement } from "../../ECSqlStatement"; | ||
| import { ECDbTestHelper } from "./ECDbTestHelper"; | ||
| import { KnownTestLocations } from "../KnownTestLocations"; | ||
|
|
| reader.step(); | ||
| }, new QueryBinder().bindId(1, "0x1")); | ||
| } | ||
| expect(prepareSpy.callCount).to.equal(1, "identical-shape queries should re-use one cached prepared statement"); |
| try { | ||
| stmt?.[Symbol.dispose](); | ||
| } catch { | ||
| // Best-effort teardown: disposing a statement whose native ECDb cache was already torn down |
Contributor
There was a problem hiding this comment.
Can we log a trace in this best effort teardown, I think we should not completely swallow errors even if they are expected.
| try { | ||
| stmt[Symbol.dispose](); | ||
| } catch { | ||
| // ignore - best effort |
| try { | ||
| stmt[Symbol.dispose](); | ||
| } catch { | ||
| // ignore - best effort |
| return { isSuccessful: true }; | ||
|
|
||
| this._stmt.bindParams(args); | ||
| this._stmt!.bindParams(args); // eslint-disable-line @typescript-eslint/no-non-null-assertion |
Contributor
There was a problem hiding this comment.
should we also just check if stmt is undef?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
closes: https://github.com/iTwin/itwinjs-backlog/issues/2239
Restores ECSQL statement caching in the synchronous
withQueryReaderpath.ECSqlRowExecutornow reuses a prepared statement obtained from the owning db's statement cache instead of re-preparing the ECSQL on every call, fixing a per-element performance regression.Changes
[_getStatementCache]()accessor onECDbandIModelDb(via a new_getStatementCachesymbol) so internal collaborators can reuse the existing prepared-statement cache.ECSqlRowExecutorretrieves/returns a prepared statement from the cache rather than compiling the ECSQL each call.IModelDbandECDbpaths.API / release notes
[_getStatementCache]is tagged@internal;common/api/core-backend.api.mdregenerated viarush extract-api. The only report delta is the two new@internalentries —withPreparedStatementkeeps its existing@public @deprecatedtags.rush changefile added with"type": "none"(lockstep monorepo, no version bump / no public API change).