-
Notifications
You must be signed in to change notification settings - Fork 122
perf(util-endpoints): reduce temporary object allocations #1957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f86da38
67e2fbc
d1b0cfd
53c7b2c
6a00f07
dfad86b
40171a4
d87e495
6e1c3a8
78eb850
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@smithy/util-endpoints": patch | ||
| --- | ||
|
|
||
| Reduce temporary object allocations |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,25 +4,28 @@ import { evaluateCondition } from "./evaluateCondition"; | |
|
|
||
| export const evaluateConditions = (conditions: ConditionObject[] = [], options: EvaluateOptions) => { | ||
| const conditionsReferenceRecord: Record<string, FunctionReturn> = {}; | ||
| const conditionOptions: EvaluateOptions = { | ||
| ...options, | ||
| referenceRecord: { ...options.referenceRecord }, | ||
| }; | ||
|
|
||
| for (const condition of conditions) { | ||
| const { result, toAssign } = evaluateCondition(condition, { | ||
| ...options, | ||
| referenceRecord: { | ||
| ...options.referenceRecord, | ||
| ...conditionsReferenceRecord, | ||
| }, | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how can we confirm no edge case relied on this? the diff seems to result in a potentially different set of objects if it can be proven this is all part of the same evaluation's lifecycle, maybe even the initial copy isn't necessary.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one thing we could do is run the full AWS endpoint test suite
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not changing the behavior. The The test needs to check the first call received an empty referenceRecord at call time, but the spy only holds a reference that's since been mutated. That's why tests had to be updated. |
||
| const { result, toAssign } = evaluateCondition(condition, conditionOptions); | ||
|
|
||
| if (!result) { | ||
| return { result }; | ||
| } | ||
|
|
||
| if (toAssign) { | ||
| conditionsReferenceRecord[toAssign.name] = toAssign.value; | ||
| conditionOptions.referenceRecord[toAssign.name] = toAssign.value; | ||
| options.logger?.debug?.(`${debugId} assign: ${toAssign.name} := ${toDebugString(toAssign.value)}`); | ||
| } | ||
| } | ||
|
|
||
| if (Object.keys(conditionsReferenceRecord).length === 0) { | ||
| return { result: true }; | ||
| } | ||
|
|
||
| return { result: true, referenceRecord: conditionsReferenceRecord }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it safe to use
optionsall the way through with no copying?Why I think it might be safe: options object lifecycle is one endpoint resolution, and this is all synchronous code...