feat(step-function): optimizer #425
Conversation
thantos
commented
Aug 19, 2022
thantos
commented
- Attempt to reduce unnecessary variables in a ASL state machine.
- Add options to turn on or off optimizations
support property assignment cases
✅ Deploy Preview for effortless-malabi-1c3e77 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| export function normalizeJsonPath(jsonPath: string): (string | number)[] { | ||
| return ( | ||
| jsonPath.match(/(\.[a-zA-Z0-9_\$]*|\[\'?[^'\]]*\'?\])/g)?.map((seg) => { | ||
| if (seg.startsWith(".")) { | ||
| return seg.substring(1); | ||
| } else if (seg.startsWith("['")) { | ||
| return seg.substring(2, seg.length - 2); | ||
| } else if (seg.startsWith("[")) { | ||
| return Number(seg.substring(1, seg.length - 1)); | ||
| } else { | ||
| throw new Error("Invalid json path: " + jsonPath); | ||
| } | ||
| }) ?? [] | ||
| ); | ||
| } |
There was a problem hiding this comment.
This scares me. Can you document the assumptions that go into this?
There was a problem hiding this comment.
Is there a json path ast we could use instead? Parse the string into its AST and process that rather than string matching?
There was a problem hiding this comment.
Good idea, was also worried about this, will try: https://github.com/dchester/jsonpath#readme
There was a problem hiding this comment.
Did a spike on this and found the results disappointing, current state works for now.
| test.each([ | ||
| ["with", undefined], | ||
| [ | ||
| "without", | ||
| { | ||
| optimization: { | ||
| joinConsecutiveChoices: false, | ||
| optimizeVariableAssignments: false, | ||
| removeNoOpStates: false, | ||
| removeUnreachableStates: false, | ||
| }, | ||
| } as ASLOptions, | ||
| ], | ||
| ])("%s optimizer", (_, opt) => { |
There was a problem hiding this comment.
Love this test.
Does this mean that all optimizations are turned on by default?
There was a problem hiding this comment.
yeah. For now at least.
export const DefaultOptimizeOptions: OptimizeOptions = {
optimizeVariableAssignments: true,
removeUnreachableStates: true,
joinConsecutiveChoices: true,
removeNoOpStates: true,
};I imagine we'll create new, experimental ones or versions of existing ones and make a breaking change to turn them on.
There was a problem hiding this comment.
Love this test.
I am glad I wrote it, found that options in SFN were borked this whole time 🤒
| // TODO: https://github.com/functionless/functionless/issues/420 | ||
| // https://mathiasbynens.be/notes/javascript-properties | ||
| test.skip("unicode variable names", () => { |
There was a problem hiding this comment.
This test is so awesome. Shame we skip it
There was a problem hiding this comment.
Just need to go the ticket to make sure it is all supported. Mostly need to normalize to valid characters in ASL.
src/asl.ts
Outdated
| * When {@link Choice} states are chained together, they can be simplified by merging the branches together. | ||
| * | ||
| * ```ts | ||
| * if(a) { | ||
| * if(b) { | ||
| * } else { | ||
| * } | ||
| * } | ||
| * | ||
| * => | ||
| * | ||
| * ```ts | ||
| * if(a && b) {} | ||
| * else(a) {} | ||
| * ``` | ||
| * | ||
| * In ASL this may remove unnecessary state transitions without any impact on the logic. | ||
| * | ||
| * Note: This could make the ASL itself larger when the same Choice is merged into multiple calling choices. | ||
| * Turn off at the cost of adding more state transitions. | ||
| */ | ||
| joinConsecutiveChoices: boolean; |
There was a problem hiding this comment.
Love it!
Do we have a test case where there are subsequent choices except the inner one has an else?
if(a) {
if(b) {}
else {}
}There was a problem hiding this comment.
(this was in there before)
We did not, added one
if (input.val !== "a") {
if (input.val === "b") {
return "hullo";
} else {
return "wat";
}
}
return "woop"; "if(input.val !== \"a\")": Object {
"Choices": Array [
Object {
"And": Array [
Object {
"Not": Object {
"And": Array [
Object {
"IsPresent": true,
"Variable": "$.input.val",
},
Object {
"And": Array [
Object {
"IsString": true,
"Variable": "$.input.val",
},
Object {
"StringEquals": "a",
"Variable": "$.input.val",
},
],
},
],
},
},
Object {
"And": Array [
Object {
"IsPresent": true,
"Variable": "$.input.val",
},
Object {
"And": Array [
Object {
"IsString": true,
"Variable": "$.input.val",
},
Object {
"StringEquals": "b",
"Variable": "$.input.val",
},
],
},
],
},
],
"Next": "return \"hullo\"",
},
Object {
"Next": "return \"wat\"",
"Not": Object {
"And": Array [
Object {
"IsPresent": true,
"Variable": "$.input.val",
},
Object {
"And": Array [
Object {
"IsString": true,
"Variable": "$.input.val",
},
Object {
"StringEquals": "a",
"Variable": "$.input.val",
},
],
},
],
},
},
],
"Default": "return \"woop\"",
"Type": "Choice",
},
src/asl.ts
Outdated
| */ | ||
| joinConsecutiveChoices: boolean; | ||
| /** | ||
| * The Functionless transpiler and sometimes user can can create states that don't do anything. |
src/asl.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Run optimizations that reduce the number of states in the graph |
There was a problem hiding this comment.
Isn't that all of them?
src/asl.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * For now, all optimizations only support a single usage of the current variable. |
There was a problem hiding this comment.
Is there an issue tracking the outstanding tasks?
