feat(step-function): optimizer - #425
Conversation
thantos
commented
Aug 19, 2022
- 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. |
sam-goodwin
left a comment
There was a problem hiding this comment.
Amazing change. Great job mr compiler man.
I'm not all the way through but here is some initial feedback.
| 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| 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.
exportconstDefaultOptimizeOptions: 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.
Uh oh!
There was an error while loading. Please reload this page.
| * 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",},| */ | ||
| joinConsecutiveChoices: boolean; | ||
| /** | ||
| * The Functionless transpiler and sometimes user can can create states that don't do anything. |
| } | ||
| /** | ||
| * Run optimizations that reduce the number of states in the graph |
| } | ||
| /** | ||
| * 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?