Fix: Model.__init__ does a per-field import inside its hot per-instance loop - #123
Merged
Merged
Conversation
![pullapprove5-fix[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ce loop Reproduced and fixed the finding. plain-postgres/plain/postgres/base.py's Model.__init__ had `from plain.postgres.fields.related import RelatedField` inside the `for field in meta.fields:` loop (line 122-123) — executed once per field per instance construction. Crucially, `RelatedField` is already imported at module level in the same file (line 30) and used by five other call sites in base.py (is_cached checks at lines 321/516, related-field filters at 884/1041/1195) — so the inline import was a pure redundant re-import with zero functional purpose (no circular-import need, nothing lazy/deferred about it). I benchmarked real Model construction (ContactSubmission with 4 fields, and Note with a ForeignKeyField/RelatedField) in the example app before and after removing the inline import, and per-instance time dropped by roughly 15-20% consistently across repeated runs, confirming the overhead was real and eliminated by hoisting nothing — the loop-local import statement was simply removed since the name was already bound at module scope. Fixed by deleting the two redundant lines from the loop body. Ran `./scripts/fix plain-postgres` (clean) and the full `./scripts/test plain-postgres` suite (795 passed, 1 skipped, unrelated) plus the example app test (1 passed) on the patched tree — no regressions. Note: this sandbox had no `uv`, Postgres, or psycopg-binary preinstalled; I installed `uv` via pip, installed PostgreSQL via apt (sudo available), and added `psycopg-binary` to make the example app runnable for benchmarking/testing — these are sandbox bootstrap steps only, not part of the committed fix.
Contributor
Author
|
Next steps:
|
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
1 participant
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.
Reproduced and fixed the finding. plain-postgres/plain/postgres/base.py's Model.init had
from plain.postgres.fields.related import RelatedFieldinside thefor field in meta.fields:loop (line 122-123) — executed once per field per instance construction. Crucially,RelatedFieldis already imported at module level in the same file (line 30) and used by five other call sites in base.py (is_cached checks at lines 321/516, related-field filters at 884/1041/1195) — so the inline import was a pure redundant re-import with zero functional purpose (no circular-import need, nothing lazy/deferred about it). I benchmarked real Model construction (ContactSubmission with 4 fields, and Note with a ForeignKeyField/RelatedField) in the example app before and after removing the inline import, and per-instance time dropped by roughly 15-20% consistently across repeated runs, confirming the overhead was real and eliminated by hoisting nothing — the loop-local import statement was simply removed since the name was already bound at module scope. Fixed by deleting the two redundant lines from the loop body. Ran./scripts/fix plain-postgres(clean) and the full./scripts/test plain-postgressuite (795 passed, 1 skipped, unrelated) plus the example app test (1 passed) on the patched tree — no regressions. Note: this sandbox had nouv, Postgres, or psycopg-binary preinstalled; I installeduvvia pip, installed PostgreSQL via apt (sudo available), and addedpsycopg-binaryto make the example app runnable for benchmarking/testing — these are sandbox bootstrap steps only, not part of the committed fix.Opened by a PullApprove implementation run (implement-finding v4) for:
Merging this is what closes them as fixed.