SchemaView performance improvements#9431
Conversation
…older Relocate SchemaView.ts, SchemaViewBinaryReader.ts and SchemaViewInterfaces.ts into a dedicated src/SchemaView/ folder and fix the relative import paths in the moved files and their importers (barrel, SchemaLocalization, test). Pure move, no behavior change - prepares the package for the incremental schema-loading work.
- Introduced `PRAGMA schema_view_fragment` to return a subset of schemas as a binary blob, enabling incremental loading. - Updated documentation for `Pragmas.md`, `SchemaView.md`, and `SchemaViewBinaryFormat.md` to reflect the new pragma and its usage. - Enhanced `getSchemaView` method to support loading only specified schemas and their dependencies, improving performance for large iModels. - Added tests for schema view fragment loading, ensuring correct behavior when loading subsets of schemas and handling dependencies. - Implemented `SchemaManifest` to manage schema references and loading order, facilitating efficient schema management.
…pragma - Added support for incremental schema loading in IModelDb using the new schema_view_fragment pragma. - Updated the SchemaView class to handle schema tokens for cache invalidation. - Modified the getSchemaView method to utilize the new incremental loading strategy. - Enhanced documentation for schema_view and schema_token pragmas to clarify their usage and benefits. - Updated tests to reflect changes in schema view lifecycle and cache invalidation logic.
…chema_token) for accuracy
…clarify failure conditions
How does this work with schema editing? Would editing a schema automatically increase the schema version? |
Not automatically. When you import an edited schema, you have to increment the version yourself or ECDb will consider the schema unchanged. It's pretty safe to assume this works for all normal scenarios, with the only exception of dynamic schemas. Our rules already state how callers should increment schema versions. Any change in non-dynamic schemas needs at least a minor digit increment. Frankly, we loosened the rules for dynamic schemas a few years ago, and I now believe that may have been a mistake. They incremented the minor version automatically very often and were worried it would eventually overflow. If this is a problem, we can add a short term fix to force invalidate the schemaView whenever dynamic schemas are involved, but feels like beyond the scope of this PR. |
…in/itwinjs-core into rschili/schema-view-fragment
…nd functionality - Simplified SchemaManifestEntry structure by replacing numeric references with direct object links to referenced schemas. - Removed unnecessary interfaces and methods related to schema row and reference row handling. - Updated SchemaManifest to build from a simpler entry specification, enhancing testability and maintainability. - Enhanced test coverage for SchemaManifest, including cases for schema closure and dependency ordering. - Renamed variables for consistency and clarity, including changing `schemaEcIdToIdx` to `schemaECIdToIdx`. - Improved documentation for methods and classes to better describe functionality and usage.
…prove performance
…in/itwinjs-core into rschili/schema-view-fragment
… and management - Introduced SchemaViewManager to handle lazy loading, incremental hydration, and cache invalidation of schema views. - Added support for fetching full and fragment schema blobs through a dedicated SchemaViewDataProvider interface. - Enhanced SchemaManifest to build schema entries and manage references from ECDbMeta query results. - Updated IModelConnection to utilize SchemaViewManager for schema view retrieval, allowing for filtered requests. - Added tests for SchemaManifest to ensure correct wiring of schema references and handling of edge cases. - Updated frontend and backend code to accommodate new schema management features and ensure compatibility with existing functionality.
…names instead of IDs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
core/ecschema-metadata/src/SchemaView/SchemaViewBinaryReader.ts:260
PropertyDefTableentries are a fixed-size 46-byte record (as the comment above notes), butvalidateCountusesminBytesPerEntry=8. On malformed/truncated blobs this can allow a much largerdefCountto pass validation and allocate an unnecessarily hugenew Array(defCount)before failing during reads. Use the actual fixed record size to bound the count and avoid large allocations on bad input.
… counts in SchemaViewBinaryReader - Updated validation counts in SchemaViewBinaryReader to use more appropriate safety margins for various tables. - Enhanced comments for clarity on validation logic in SchemaViewManager. - Expanded documentation for `PRAGMA schema_view_fragment` to clarify schema dependency requirements.
grigasp
left a comment
There was a problem hiding this comment.
Reviewed public api, docs and example code - looks good, I think the change should help with the problems we were seeing. I did not look at the implementation details.
| Sometimes, these schemas can grow quite large and complex. It is possible to have a hundred schemas, thousands of classes and hundreds of thousands of properties flat, which expands to millions of properties when you include inherited properties. | ||
|
|
||
| It lives in `@itwin/ecschema-metadata` and should be the first choice for accessing schema metadata at runtime - for example in presentation rules, property grids, or data-driven UI. | ||
| When that happens, performance and memory consumption suffer, plus in TypeScript there is the additional caveat of our metadata library loading the info synchronously, which blocks the backend. A web service may freeze for multiple seconds, which is not acceptable. |
There was a problem hiding this comment.
by "our metadata library" you mean the SchemaContext implementation, correct? Either way, I'm not sure we want to mention this drawback - IMO it's an implementation detail which doesn't need to be documented and could potentially be fixed in the future.
There was a problem hiding this comment.
you're right, I removed that part, it doesn't really seem to add much value.
| "changes": [ | ||
| { | ||
| "packageName": "@itwin/ecschema-metadata", | ||
| "comment": "add schema fragments feature and abstraction manager layer for schemaview", |
There was a problem hiding this comment.
The changelog comments should be user friendly messages with markdown syntax. Should start with uppercase letter, code in backticks, etc. And should be understandable by someone who's only familiar with the public api (at best).
There was a problem hiding this comment.
Markdown? I adjusted the message but kept it short. Are you saying this should include detailed examples, too? That seems redundant to NextVersion.md and the documentation that is included.
…in/itwinjs-core into rschili/schema-view-fragment
hl662
left a comment
There was a problem hiding this comment.
I think there's a lot of bloat in terms of in line comment that can be improved, deduped
| return; | ||
| try { | ||
| const liveToken = await this._dataProvider.fetchSchemaToken(); | ||
| if (liveToken === existing.schemaToken || this._viewPromise !== existingPromise) |
There was a problem hiding this comment.
This identity check can skip a required invalidation: a concurrent cached getSchemaView() replaces _viewPromise but still resolves to the same stale view, so a changed schema token is ignored. Maybe queue invalidation on the same serialized chain as loading, and add a regression test that pauses token fetching, performs a concurrent cache read, then returns a changed token and verifies the old view is discarded?
There was a problem hiding this comment.
updated the code and added a test as you suggested
| * faster and lower-memory than [[schemaContext]]. Use [[schemaContext]] for schema authoring, | ||
| * custom-attribute deserialization, or anywhere you need the full ecschema-metadata object graph. | ||
| * | ||
| * Pass `args.schemas` to request only a subset (plus its references) instead of every schema. |
There was a problem hiding this comment.
This duplicates the schemas property documentation, and the method already links to GetSchemaViewArgs. Can we remove it and keep this method comment focused on behavior that is not documented by an individual argument, such as the shared accumulating view and cache lifecycle?
There was a problem hiding this comment.
the method doc now only states what the arguments don't
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
core/ecschema-metadata/src/SchemaView/SchemaViewBinaryReader.ts:310
- When merging fragments, the schema ecInstanceId -> schemaIdx map is updated unconditionally. If a schema is ever included twice (e.g., native writer/provider bug, or an accidental duplicate merge), this will silently overwrite the mapping and can corrupt subsequent cross-reference resolution. Consider failing fast if an ecInstanceId was already merged.
…tion check in SchemaViewManager as per review comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
core/ecschema-metadata/src/SchemaView/SchemaViewBinaryReader.ts:263
- The blob layout is being changed in-place (e.g.,
primitiveTypeis now read asuint16), but the headerversioncheck still requiresschemaViewFormatVersion === 1. This means a v1 blob produced by an older backend/native (whereprimitiveTypewasuint8) will still advertiseversion=1and will be mis-parsed instead of failing cleanly. That undermines the frontend’s version-pinning strategy (PRAGMA schema_view(v1)/schema_view_fragment('v1;...')) for talking to older/newer backends and can lead to hard-to-diagnose runtime failures under version skew.
Consider introducing a new blob format version (e.g. bump schemaViewFormatVersion to 2) and keeping the old v1 reader/writer behavior for compatibility, or otherwise ensure the reader can detect and reject the legacy v1 layout before interpreting it as the new layout.
imodel-native: iTwin/imodel-native#1479
Fixes: #9430
What
SchemaViewwas built and tuned against a very large iModel, balanced for the general mix of use cases.Presentation has been testing it for adoption and the results are promising in almost every scenario, but
one critical scenario regresses on very large iModels - enough that they cannot adopt
SchemaViewas-is.The scenario
That path only needs BisCore, yet
SchemaViewpaid to hydrate every schema in the file before returning. This PR makesSchemaViewpay only for what the caller asks for, and removes a fixed cost that hurt every consumer.Two changes, independent but shipped together.
Change 1: a cheaper schema-identity token (
PRAGMA checksum(schema_token))SchemaViewuses a token to detect when an iModel's schemas have changed so it can drop its cached view.It was obtaining that token from
PRAGMA checksum(ecdb_schema), a SHA3 hash over the full contents of all schema tables (every class, property, and custom-attribute instance).On the large iModel above, that checksum alone takes ~1.7 seconds, almost entirely CPU-bound on the hash - while loading BisCore plus its references via the binary blob is only ~30 ms. The token, meant to be a cheap "did anything change?" check, was dominating the whole operation, and every
SchemaViewconsumer paid it regardless of how much schema they needed.This PR adds
PRAGMA checksum(schema_token), which hashes schema identity only - the name and version of each schema (one row per schema inec_Schema, ordered by name). It is essentially free. The same cheap hash now also backs theschemaTokencolumn returned byPRAGMA schema_viewand the newPRAGMA schema_view_fragment, so a cached view and any later token check derive from the same value.Measured on the ~30 GB / ~100-schema file: the token computation dropped from ~1744 ms to 1 ms, and the schema fragment pragma (see Change 2 below) from ~1759 ms to ~14 ms (cold ≈ warm, confirming the cost was CPU, not disk).
Known limitation (accepted): because the token hashes name + version only, it does not detect a schema whose contents change without a version bump. ECDb only allows in-place re-import for dynamic schemas, so that is the only case affected; We accept this for now and can strengthen the hash later (for example a cached per-schema content checksum) without changing the pragma's contract.
Frontend
IModelConnection.invalidateSchemaViewIfChangednow queriesPRAGMA checksum(schema_token)(reading.token) instead ofPRAGMA checksum(ecdb_schema)(.sha3_256).Change 2:
getSchemaView({ schemas })- load only a subsetgetSchemaView()gains an optional argument. It is purely additive:Behavior:
their closure into the same view, so schemas loaded earlier stay available.
findClassand friends returnundefined. Cross-schema walks (derivedClasses, etc.) are complete only over what is currently loaded.How it works
Manifest (cheap reference graph). On first subset request, the backend reads the schema reference graph from ECDbMeta (
meta.ECSchemaDef+meta.SchemaHasSchemaReferences) - just names, versions, ids, and reference edges, no schema data. This is exposed as the newSchemaManifesttype.This has one incoming path via
getSchemaView:Serialized merges. Overlapping concurrent requests are serialized behind one in-flight promise and re-check the loaded-set inside the continuation, so two requests can never double-merge a schema.
Fixed Primitive Type enum width
The binary blob incorrectly used 8 bits for the primitive type, but we need 16 bits for it. I decided to fix this format in-place. The API is still beta, and the addon + backend are coupled together, so both should always speak the same language.
A point could be made that this warrants a v2 binary blob format - in the future such changes probably should, however, since no real consumer on frontend picked this up yet, and we will backport, I believe it's safe to fix this in place. It's a fix for the v1 format, not an evolution of it.
Performance numbers:
Ran this on a local release addon build against 2 very large iModels
First iModel (32 GB, 101 schemas, ~221k properties)
BisCoreonly): load + iterate~54x faster.
Second iModel (9.4 GB, 33 schemas, ~117k properties)
~33x faster.