Native Bulk Element Deletion API#9052
Conversation
…/iTwin/itwinjs-core into rohitptnkr/bulk-element-deletion
rschili
left a comment
There was a problem hiding this comment.
I approve, but those performance numbers make me wonder if this is really good. The gain is not big, and the slowdown with <50 deletes is significant. Maybe the typescript deleteElements method could check the size of the provided ID set and then call one or the other internally?
Yeah, the performance numbers are a little unfair to be honest. My plan was to ask the users to try out this beta which does everything on the native side instead of their TS tool and share their findings on the performance. |
Co-authored-by: Travis Cobbs <77415528+tcobbs-bentley@users.noreply.github.com>
…/iTwin/itwinjs-core into rohitptnkr/bulk-element-deletion
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…/iTwin/itwinjs-core into rohitptnkr/bulk-element-deletion
|
Is there a new entry on NextVersion.md that perhaps mentions this is experimental and encourage users to try using it instead of our existing deleteElement API? |
hl662
left a comment
There was a problem hiding this comment.
Reviewed the deleteElements bulk deletion API across four phases (API surface, core logic, tests, documentation). Overall this is a solid addition — strong test coverage for cascading, constraint checking, and edge cases. A few items below, roughly ordered by importance.
File-level notes (no specific line):
- All tests go through
EditTxn.deleteElements— the publicIModelDb.Elements.deleteElementswrapper (with its own input validation, cache eviction, and return-set construction) isn't exercised directly. A small integration test callingiModelDb.elements.deleteElements(...)would cover that path. - The
ModelSelectorRefersToModelslink-table cleanup is well covered — any appetite for a similar test withElementRefersToElementsorElementDrivesElement? The learning doc mentions those are cleaned up too, but there's no test verifying it.
…/iTwin/itwinjs-core into rohitptnkr/bulk-element-deletion
imodel-native: iTwin/imodel-native#1344
I have added performance tests comparing deleteElement (old) vs deleteElements (new) and deleteDefinitionElements (old) vs deleteElements (new).
The new APIs were expected to do additional validations like checking for intra and inter set FK violations, auto-handling parent child deletions within the same call, expanding sub-models if a modeling element is being deleted, making the deletions element-order-agnostic (all of which are not handled by the old API calls).
Performance stats from the new full-stack-tests perftest added:
When definition elements exist in the delete set, we run checks to ensure there is no element which is being used, along with validations for FK violations, parent-child and sub-models cascade delete support.
This gives us a better deletion confidence and a reduction in API call failures, but at the expense of a performance hit.
Adding to that are the domain handler callbacks that need to be called before and after deleting every element, which take up a chunk of the processing time.
Hence, the validations add significant overhead to the running time.
A more realistic performance check will be to run the new Api against the internal tool that the user has written to do the validations and compare the performances.
@khanaffan, will you please take a look at the tests and the native API ?