perf(core): drop a redundant deepcopy of command kwargs - #7607
Open
dexhunter wants to merge 1 commit into
Open
Conversation
StaticCommandRunner._execute_func deep-copied kwargs, popped "chart" from the original, then deep-copied again. The first copy was overwritten before it was ever read, so every command paid two full deep copies of the caller's payload where one was needed. For commands that take a data payload - technical, econometrics and quantitative all accept list[Data] - that is the whole dataset copied twice per call. Removing the dead store halves it: with a 1,000-row payload the copy step goes from 5.83 ms to 2.88 ms, and with 5,000 rows from 30.5 ms to 14.6 ms. Behaviour is unchanged: the removed assignment was dead.
Author
|
A note on why this PR shows no checks at all, because that is easy to read as "nothing has happened here yet". No workflow has ever run on it. Every job is held in Since CI could not speak for it, I ran the equivalent locally against
Happy to fold this into a larger cleanup of that function if you would rather not spend a review on a one-line change, or to close it if the duplication is deliberate and I have misread it. |
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.
StaticCommandRunner._execute_funcdeep-copieskwargs, pops"chart"from the original, then deep-copies again:The first copy is never read — it is overwritten two lines later — so every command dispatch pays two full deep copies of the caller's payload where one is needed. This removes the dead store.
What it costs today
The copy is of
kwargs, so the cost tracks the payload. Forobb.equity.price.historical(...)-style commands the kwargs are a handful of scalars and the redundant copy is 2.7 µs — genuinely irrelevant next to the provider HTTP call, and I would not have opened a PR for that alone.It matters for the commands that take a
datapayload —technical,econometricsandquantitativeall acceptlist[Data]— because there the payload is the dataset and the computation is local, so there is no network call to hide behind. Measured on the region itself, median of 15 samples per round, 3 interleaved rounds:Frequency is one per command dispatch, unconditionally — it is not behind the
chartbranch or any provider check. It also runs synchronously insideasync _execute_func, so under the API server it occupies the event loop for that whole duration.A control confirms where the cost is: keeping both copies but moving the
popfirst measures 5.87 ms at 1,000 rows, i.e. unchanged. Thepopis not the cost; the second copy is.Behaviour
Unchanged. The removed line's value was overwritten before any read, so the surviving copy — taken after
"chart"is popped — is exactly what the rest of the function already used.openbb_platform/core/tests/app/test_command_runner.py: 25 passed, 1 failed. That one failure,test_static_command_runner_chart, reproduces identically on unmodifieddevelopat3e071fccin the same environment, so it is pre-existing and not from this change. I did not touch any test.What I deliberately left out
Several alternatives are faster than this patch and I am not proposing them:
json(1.78 ms) orpickle(0.59 ms) round-trips — both change which payload types survive;datakey, or a shallow copy, or copying only when charting was requested (0.001–0.002 ms) — all alias the caller's data, so a downstream mutation could leak back;deepcopycalls in this file, atmerge_args_and_kwargs, are not dead — their values flow intoparameter_map— so removing them would be a real semantic change rather than a cleanup, and they are out of scope here.This PR is only the dead store.
Exploration record
Twelve variants of this region were implemented and measured, including the ones above that do not qualify and a no-copy floor reference. Three structurally different ways of expressing "copy once" — deleting the dead store, popping from the copy, and a single copy with an explicit memo — land at 2.881 / 2.860 / 2.858 ms, within 0.8% of each other, so there is one improving move here rather than a spectrum. Public record of that exploration: https://dashboard.weco.ai/share/DJSi5Z0pwQGnRZN3ZJhlFAdbCs8GHIX5