Do a better job preventing serialization of unnecessary objects in closure serialization #1543
Conversation
…osure serialization.
| // logInfo = logInfo || func.name === "addHandler"; | ||
| if (logInfo) { | ||
| console.log("Serializing " + func.name); | ||
| } |
There was a problem hiding this comment.
this is effectively debug-only code. I end up adding this in a bunch, and i wouldn't mind just leaving this in. Note that it will not do anything by default since logInfo=undefined. But this allows for a pretty easy way to opt into turning on printf-style logging when helpful.
We could also consider some sort of logging library, but i didn't want to go through the pain of doing that.
| serialize: (o: any) => boolean): FunctionInfo { | ||
| serialize: (o: any) => boolean, logInfo?: boolean): FunctionInfo { | ||
|
|
||
| // logInfo = logInfo || func.name === "addHandler"; |
There was a problem hiding this comment.
Useful for being able to turn on logging when serializing just a specific function.
There was a problem hiding this comment.
Should probably leave this out or formalize it as part of the debug logging for this package.
| // if this property was invoked or not. | ||
| for (const existingProp of combined) { | ||
| if (existingProp.name === current.name) { | ||
| existingProp.invoked = existingProp.invoked || current.invoked; |
There was a problem hiding this comment.
i removed this merging logic as it was too complex now that we have 'chains' of property-accesses recorded. Instead, now we just record all the chains, and we consider all of them (including dupes) when doing closure serialization. so if we have "a.b.c()" and "a.b.c" and "a.b.c", we'll record three chains of 'b.c' (with the first saying it is invoked). Then, in closure serialization, when we're emitting the value for 'c', we'll see if any of the accesses were 'invoked'.
There was a problem hiding this comment.
Couple notes. Generally this looks good - though there's a lot of subtle interactions here that are a little hard to track through. Glad to see the new tests!
Possibly @swgillespie can take a look as well.
| serialize: (o: any) => boolean): FunctionInfo { | ||
| serialize: (o: any) => boolean, logInfo?: boolean): FunctionInfo { | ||
|
|
||
| // logInfo = logInfo || func.name === "addHandler"; |
There was a problem hiding this comment.
Should probably leave this out or formalize it as part of the debug logging for this package.
| // validate our invariants. | ||
| for (const chain of localCapturedPropertyChains) { | ||
| if (chain.infos.length === 0) { | ||
| throw new Error("We should never have gotten an empty chain."); |
There was a problem hiding this comment.
Nit: "Expected a non-empty chain." or similar.
| context: Context, | ||
| serialize: (o: any) => boolean): Entry { | ||
| serialize: (o: any) => boolean, | ||
| logInfo: boolean | undefined): Entry { |
There was a problem hiding this comment.
I like forcing people to have to remember to call it. logInfo?:boolean means it is optional, which means you might forget it.
There was a problem hiding this comment.
Does typescript not consider T | undefined arguments to be optional?
Serialize doNotCapture functions with a throwing function on the runtime side. This will help people get a reasonable error at runtime instead of just a null-ref.
|
I took a 3-pronged attack here:
|
I recommend reviewing https://github.com/pulumi/pulumi/pull/1543/files?w=1 as this will make the indentation changes tolerable.
I also recommend looking at parseFunction.ts first, then createClosure.ts.