Conversation
There was a problem hiding this comment.
Sorry @droideck, your pull request is larger than the review limit of 150000 diff characters
847b699 to
673677e
Compare
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
6e3cb5e to
b46a74d
Compare
|
I find the idea/implementation excellent. However I wonder if it could not be extended. Something we could extend is that assuming that there are very often a very large set of candidate (e.g. >1000) we could systematically normalize the assertion (only for equality ava) in the filter. Even for AND, NOT lists. So that we normalize filter assertion once instead of 1000 times. I think it could be extended in a separated ticket, no need to make this PR too complex. But this PR should be compatible with future extension. WDYT |
|
@tbordaz, agreed, it's worth exploring! One correction though... Assertion normalization is already done once per search, for the whole filter tree, not just the OR. But I think the idea is sound and we still have things to do, possibly. And one huge thing which I'd keep as a separate optimization, I think... Filters outside the search path (ACI targetfilter, roles, psearch, plugin configs) really do renormalize assertions per evaluation, because those trees never get the normalize pass. Extending normalize-once there would be nice! But those trees are shared, long-lived, and evaluated concurrently (and normalization mutates the value in place) so each consumer needs its own ownership story (ACL normalizing at ACI-parse time would be the first candidate, IMO). So yeah, I think it's certainly a separate ticket territory. This PR should be okay as is... The lookup table consumes the normalize pass instead of doing its own (it borrows the already-normalized bytes and doesn't touch the tree after the build), and it only ever lives on the backend's per-search filter copy. So whatever we end up changing about where normalization happens, the table just picks up whatever values are there, and it can't collide with the shared trees we'd need to touch for the ACI/roles part. |
6fe89d0 to
d87f192
Compare
Description: The per-entry filter test walks every OR branch for every candidate entry, and each branch re-normalizes the entry's values through the syntax plugin, so a filter like (|(uid=v1)...(uid=v1000)) costs candidates x branches even when fully indexed. The time is there, not in candidate generation: the same 1000-branch OR costs 7 ms when 999 values are absent and over a second when all are live. Build a sorted table of the branch values once per search, on the backend's private normalized filter copy, and let each entry find its branch with one normalization and a binary search. The table only selects which branch to test: a hit still runs the same access check and match call as the classic walk, all-miss entries are decided directly where false and undefined are indistinguishable (no NOT ancestor, not VLV), and anything the table cannot decide exactly falls back to the untouched walk. Eligibility is narrow: at least 16 equality branches on one attribute, a string-family or DN syntax whose equality is byte equality of the normalized forms, a standard matching rule, and no attribute options. The largest qualifying family wins, first occurrence breaking ties. nsslapd-enable-or-filter-lookup (default on) turns the feature off. On a 100,000-entry database 10,000-candidate search drops from 23 s to 0.4 s, with no regression beyond noise on ineligible shapes. Also fix a latent double free in list_candidates: idl_set_insert_idl frees an ALLIDS list it is handed, so stop keeping a local alias to it. Coverage: result parity across matching rules, ACLs, paging, and VLV; strict log-based feature contracts; an ASan filter-lifecycle module. Fixes: 389ds#7664 Relates: 389ds#6275 Assisted by: Claude (investigation and tests) Reviewed by: ?
Skip the table build when the candidate list is empty: no entry would ever probe it, so it was pure waste. Resolve each branch's family and key length in a single pass: DN key validation is the expensive part of the build, and every DN branch paid it twice. Copy probe values into a stack buffer: the heap round trip cost a malloc/free per value per candidate. Add a DN-syntax family feature test: the DN build path had only result-parity coverage, no log-contract coverage. Re-arm the CoS test's poll with a no-op modify: a cos cache notification that lands during an in-flight rebuild is dropped, and nothing else re-triggers the rebuild.
d87f192 to
229ddfc
Compare
Description:
The per-entry filter test walks every OR branch for every candidate entry, and each branch re-normalizes the entry's values through the syntax plugin, so a filter like (|(uid=v1)...(uid=v1000)) costs candidates x branches even when fully indexed. The time is there, not in candidate generation: the same 1000-branch OR costs 7 ms when 999 values are absent and over a second when all are live.
Build a sorted table of the branch values once per search, on the backend's private normalized filter copy, and let each entry find its branch with one normalization and a binary search. The table only selects which branch to test: a hit still runs the same access check and match call as the classic walk, all-miss entries are decided directly where false and undefined are indistinguishable (no NOT ancestor, not VLV), and anything the table cannot decide exactly falls back to the untouched walk.
Eligibility is narrow: at least 16 equality branches on one attribute, a string-family or DN syntax whose equality is byte equality of the normalized forms, a standard matching rule, and no attribute options. The largest qualifying family wins, first occurrence breaking ties. nsslapd-enable-or-filter-lookup (default on) turns the feature off.
On a 100,000-entry database 10,000-candidate search drops from 23 s to 0.4 s, with no regression beyond noise on ineligible shapes.
Also fix a latent double free in list_candidates: idl_set_insert_idl frees an ALLIDS list it is handed, so stop keeping a local alias to it.
Coverage: result parity across matching rules, ACLs, paging, and VLV; strict log-based feature contracts; an ASan filter-lifecycle module.
Fixes: #7664
Relates: #6275
Assisted by: Claude (investigation and tests)
Reviewed by: ?