Improve PartialECChangeUnifier to handle very large changeset #7607
Conversation
…attbjordan/disk-based-unifier-poc
…om/iTwin/itwinjs-core into mattbjordan/disk-based-unifier-poc
| // table name should be unique in case two PartialECChangeUnifiers are made | ||
| this._db.withPreparedSqliteStatement(`CREATE TABLE [temp].[${this._cacheTable}] ([key] text primary key, [value] text)`, (stmt) => { | ||
| if (DbResult.BE_SQLITE_DONE !== stmt.step()) { | ||
| // throw new Error(db.nativeDb.getLastError()); |
There was a problem hiding this comment.
remove commented out line
| private dropTempTable(): void { | ||
| this._db.withPreparedSqliteStatement(`DROP TABLE IF EXISTS [temp].[${this._cacheTable}]`, (stmt) => { | ||
| if (DbResult.BE_SQLITE_DONE !== stmt.step()) { | ||
| // throw new Error(db.nativeDb.getLastError()); |
There was a problem hiding this comment.
remove commented out line
| stmt.bindString(1, key); | ||
| stmt.bindString(2, JSON.stringify(shallowCopy)); | ||
| stmt.step(); | ||
|
|
| const key = PartialECChangeUnifier.buildKey(rhs, db); | ||
| const lhs = this._cache.get(key); | ||
| const key = this.buildKey(rhs); | ||
| const lhs = this.getInstance(key); // get from temp db instead |
There was a problem hiding this comment.
comment is probably not needed anymore
| } | ||
| private static replaceBase64WithUint8Array(row: any): void { |
There was a problem hiding this comment.
add newline between methods
| } | |
| private static replaceBase64WithUint8Array(row: any): void { | |
| } | |
| private static replaceBase64WithUint8Array(row: any): void { |
| } | ||
| private dropTempTable(): void { |
There was a problem hiding this comment.
add newline between methods
| } | |
| private dropTempTable(): void { | |
| } | |
| private dropTempTable(): void { |
| } | ||
| /** |
There was a problem hiding this comment.
add newline between methods
| } | |
| /** | |
| } | |
| /** |
| } | ||
| private *getInstances(): IterableIterator<ChangedECInstance> { |
There was a problem hiding this comment.
add newline between methods
| } | |
| private *getInstances(): IterableIterator<ChangedECInstance> { | |
| } | |
| private *getInstances(): IterableIterator<ChangedECInstance> { |
| } | ||
| /** |
There was a problem hiding this comment.
add newline between methods
| } | |
| /** | |
| } | |
| /** |
| private _readonly = false; | ||
| public constructor(private _db: AnyDb) { | ||
| this.createTempTable(); |
There was a problem hiding this comment.
What do you think about turning using a temp table into an option? Because this method should be quite a bit slower than the old method of just keeping the values in a map in RAM.
Maybe just a boolean parameter, then the question is which should be the default? Only stupidly large iModels/Changesets will have this issue - but those aren't THAT uncommon or unexpected. It'd be nice if we could automatically make an educated guess for which method to use based on the size of the iModel and changes - but is that feasible here?
Fixes: #7553
This API is beta.
PartialECChangeUnifieruse in memory cache that resulted OOM error when processing very large changeset.Solution is to use SQLite TEMP db to unify changes across multiple tables
original POC done by @mattbjordan