right now, VersioncontrolBranch::delete() with the 'nested' option will load ALL commits on that branch and iterate over them in PHP to see if they are on any other branches, and if not, then delete them. that means a potentially *huge* number of objects for large repositories, and we'd just be *asking* for memory exhaustion.
we need to see if we can optimize that logic so that the only commits that get loaded in the first place are the ones that need to be deleted. better to have one potentially expensive query than suddenly spike to 500MB of mem usage.
marking this major, as it is a nontrivial issue in making our syncEvent() logic fully accurate.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 0001-Issue-1642310-Optimize-logic-for-branch-deletion-wit.patch | 3.63 KB | marvil07 |
| #5 | interdiff.txt | 2.8 KB | marvil07 |
| #2 | 0001-Issue-1642310-Optimize-logic-for-branch-deletion-wit.patch | 2.33 KB | marvil07 |
Comments
Comment #1
marvil07 commentedI'll providing a patch soon.
Comment #2
marvil07 commentedThe patch. It works for some test cases I have locally.
@sdboyer: Do you mind taking another look at it before adding it to mainline?
Comment #3
sdboyer commentedi would prefer that we pass the call through the API rather than directly querying that table, if *at all* possible. so, adding a conditional on the loadCommits() call which adds the HAVING COUNT, etc. logic.
breaking that encapsulation creates technical debt later for any backend that wants to use an alternate storage mechanism. which, eventually, i do.
if it proves too difficult, we can go with this.
Comment #4
sdboyer commentedComment #5
marvil07 commentedI agree, breaking the encapsulation is a bad idea.
The problem that makes it not possible to do it through the controller load(), is that the query that has the group by clause is not on the base table(it's on versioncontrol_operation_labels).
The only way I see to solve this naturally, is to add a "VersioncontrolOperationLabels" entity and with it also an "VersioncontrolOperationLabelsController", so it is encapsulated there. But I am not convinced of that, since it is not really an entity as all the others we have.
So, I end up adding the custom queries on a special method at operation controller, so, it can be customized on other controllers.
I have added the attached patch to both 7.x-1.x and 6.x-2.x
Comment #6
marvil07 commented