Include part fields in the KiCad category listing (one request per category instead of one per part) - #1489
Open
arlenarlenarlen wants to merge 1 commit into
Conversation
KiCad's symbol chooser makes one HTTP request per part when it enumerates an
HTTP library. SCH_IO_HTTP_LIB::EnumerateSymbolLib() back-fills every part from
parts/{id}.json unless the category listing already carried a "fields" object,
which HTTP_LIB_CONNECTION::SelectAll() uses to set detailsLoaded (KiCad >=
10.0.5, common/http_lib/http_lib_connection.cpp).
getCategoryParts() returns only id/name/description, so that skip never
triggers. On a 351-part instance that is 366 requests per cold library open
instead of 15. Since most of each request is connection setup and round trip
rather than server time, it dominates: measured on that instance, 2.4s of
server time and tens of seconds of wall clock, against 0.2s and a few seconds
with the fields inlined. The response also gets slightly SMALLER in total
(302 KiB vs 331 KiB), because 14 batched responses carry less overhead than
351 individual ones.
This wires up the $minimal argument that getCategoryParts() already accepts
and that KiCadApiController already exposes as ?minimal=1. It currently has no
effect on the response: the cache closure captures only $category, so the flag
changes the cache key and nothing else. Now ?minimal=1 keeps the light
id/name/description shape, and the default returns the same record the
per-part endpoint returns, which is a superset of it.
Refs Part-DB#1464
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
getCategoryParts()returns onlyid/name/description, so KiCad's symbol chooser has to fetch every part individually when it enumerates the library. This inlines the recordgetKiCADPart()already builds, which lets KiCad skip those requests.Why
SCH_IO_HTTP_LIB::EnumerateSymbolLib()back-fills each part fromparts/{id}.jsonunless the category listing already carried afieldsobject —HTTP_LIB_CONNECTION::SelectAll()uses its presence to setdetailsLoaded(KiCad ≥ 10.0.5,common/http_lib/http_lib_connection.cpp). Because the listing omits it, the skip never triggers.On the 351-part / 14-category instance I measured, that's 366 requests per cold library open instead of 15. Most of each request is connection setup and round trip rather than server time — for that instance, ~105 ms TCP+TLS and ~50 ms RTT against ~60 ms of server work — so the request count dominates and a cold open takes tens of seconds.
Measured on the same database, same disk, cold cache both times:
The total payload gets smaller: 14 batched responses carry less overhead than 351 individual ones. The largest category costs 45 ms cold to assemble, and the result is cached with the same
Part/Category/Footprinttags as before, so it is rebuilt exactly as often as it was.How
By wiring up the
$minimalargumentgetCategoryParts()already accepts and thatKiCadApiControlleralready exposes as?minimal=1. Today that flag has no effect on the response — the cache closure captures only$category, so$minimalchanges the cache key and nothing else. After this change:?minimal=1→ the lightid/name/descriptionshape (what the endpoint returns for everyone today),So it also gives users the control asked for in #1464, and keeps a cheap listing available for clients that only need names.
Tests
Three cases added to
tests/Services/EDA/KiCadHelperTest.php, using the existingEDADataFixtures(category 1 has a KiCad symbol, so its parts are EDA-visible): the default listing carriesfieldsplussymbolIdStr,?minimal=1still returns exactlyid/name/description, and both listings describe the same parts in the same order.Checks run locally
On PHP 8.4 + SQLite (a cell of the CI matrix), in a container with the extensions CI installs:
bin/phpunit(full suite)bin/phpunit --filter KiCadHelperTestcomposer phpstan(level 5)bin/console lint:yaml config --parse-tagsbin/console lint:twig templates --env=prodbin/console doctrine:schema:validate --skip-syncNot run: the MySQL and PostgreSQL matrix cells, PHP 8.2/8.3/8.5, and Codecov upload. The change builds a PHP array and issues no new queries, so it should be database-agnostic, but I didn't verify that empirically.
Two notes on the tooling while I was in there, both pre-existing and unrelated to this PR:
vendor/bin/ecscan't run:ecs.phpuses the deprecatedreturn function (ECSConfig $ecsConfig)format, and the installed Easy Coding Standard passes anECSConfigwhere the closure's signature demands aContainerConfigurator, so it fails while building its container before analysing anything. Happy to send a separate PR convertingecs.phpto the fluent API if that's wanted. I matched the surrounding style inKiCadHelper.phpby hand instead.CONTRIBUTING.mdsays phpstan runs at--level=2; the actualcomposer phpstanscript andphpstan.dist.neonboth say level 5. I used level 5.Refs #1464 — this covers the listing side of that request (a light vs. full listing); it does not add per-parameter selection.