Skip to content

find() fetches the document instead of searching for it - #18

Closed
Spamercz wants to merge 2 commits into
v2.0from
perf/find-by-document-get
Closed

find() fetches the document instead of searching for it#18
Spamercz wants to merge 2 commits into
v2.0from
perf/find-by-document-get

Conversation

@Spamercz

@Spamercz Spamercz commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

EntityManager::find() built a term query on _id and ran it through findOneBy, so looking a document up by its identifier cost a full search — query parsing, scoring, a fan-out across shards — to return the one document a GET returns directly.

It matters more than it looks, because find() is not only called by application code. EntityFactory::resolveProperties() resolves every single-entity reference through it while hydrating, one per reference:

} else {
    $propertyValue = $entityManager->find(id: $value, class: $propertyTypeName);
}

A page hydrating twenty entities with a handful of references each pays for hundreds of searches that were only ever document lookups. In the application this was measured in, that page sits at p50 ~880 ms while the query driving it takes 26 ms.

Two commits

1. Get raises DocumentNotFound for a missing document.

Every client exception was wrapped as ElasticSearch, so a document that is not there and a cluster that cannot be reached arrived as the same type. Meanwhile findOneBy() has always raised DocumentNotFound for that exact situation — and AbstractBaseService::get() means to as well, but its found() check cannot be reached: the blanket wrap turns a 404 into ElasticSearch, and its own catch rethrows that first.

Four tests asserted the imprecise type. Their intent — that asking for a deleted document fails — is unchanged; they now name which failure it is. Indices\Get is untouched, it is a different class with a different question.

2. find() uses it. Nothing needs catching for the missing case: DocumentNotFound is what find() already propagated through findOneBy.

Verification

The suite has heavy inter-test pollution — whole files flip between pass and fail across runs, so totals are not comparable. Running each test file in isolation with a clean Elasticsearch between files:

failing files
baseline (v2.0) 13
with this change 13

Empty diff in both directions. The 13 are pre-existing. make phpstan clean.

What I got wrong first

My first attempt added a missingIsAnAnswer boolean to Get so it could return found() === false instead of throwing. That was a wart, and it was pointed out that this library signals missing things with exceptions throughout. It does — and the right fix was not a second style but a more precise exception, which also revealed the unreachable found() branch above. The flag is gone.

Note for the reviewer

make cs fails on this branch and on v2.0 untouched — core.autocrlf=true leaves CRLF in the working tree for files nobody edited (EventManager.php, EntitySettingsLocator.php). Committed content here is LF; verified with git show :file | file -.

Not in this PR

The bigger win is fewer round trips rather than cheaper ones — batching reference resolution into an mget per class before hydration, so find() hits the identity map and does no I/O at all. That is a larger change to EntityFactory and deserves its own review.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LiMwcctmVX9zeodkjmoTnK

Spamercz and others added 2 commits September 2, 2026 14:36
Every client exception was wrapped as ElasticSearch, so a document that is not
there and a cluster that cannot be reached arrived as the same type - while
findOneBy() has always raised DocumentNotFound for that very situation, and
AbstractBaseService::get() means to as well. Its found() check cannot be reached
today: the blanket wrap turns a 404 into ElasticSearch, and its own catch
rethrows that first.

Four tests asserted the imprecise type. Their intent - that asking for a deleted
document fails - is unchanged; they now name which failure it is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LiMwcctmVX9zeodkjmoTnK
…g for it

find() built a term query on _id and ran it through findOneBy, so looking a
document up by its identifier cost a full search - query parsing, scoring, a
fan-out across shards - to return the one document a GET returns directly.

It matters because find() is not only called by application code. EntityFactory
resolves every single-entity reference through it while hydrating, one per
reference, so a page hydrating twenty entities with a handful of references each
pays for hundreds of searches that were only ever document lookups.

Nothing needs catching for a missing document: Get now raises DocumentNotFound,
which is what find() already propagated through findOneBy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LiMwcctmVX9zeodkjmoTnK
@Spamercz
Spamercz force-pushed the perf/find-by-document-get branch from 8c85804 to 0c1e06b Compare September 2, 2026 12:36
@Spamercz

Spamercz commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Closing unmerged. Measured against production data, this saves ~0.20 ms of Elasticsearch time per reference — around 70 ms of an ~880 ms page — because a term search on _id and a document GET are both sub-millisecond server-side. The cost is the 350 round trips per page, not what each one does, and this removes none of them.

The eager loading it was meant to help is untouched: EntityFactory still resolves every single-entity reference through find() during hydration. Parking rather than merging an 8% change that reads like a fix.

If picked up later, the approach worth taking is batching — collect the reference (class, id) pairs in createCollection before hydrating, resolve them with one mget per class, and seed the IdentityMap so find() does no I/O at all. The DocumentNotFound precision fix in the first commit stands on its own if it is ever wanted separately.

@Spamercz Spamercz closed this Sep 2, 2026
@Spamercz
Spamercz deleted the perf/find-by-document-get branch September 2, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant