[perf]: don't instantiate debugger info if not needed #331
Conversation
jamieQ
force-pushed
the
jquadri/conditional-dbg-info
branch
from
be86bd7 to
092977f
Compare
| workflowType: "\(WorkflowType.self)", | ||
| kind: .didUpdate(source: .external) | ||
| ) | ||
| debugInfo: context.ifDebuggerEnabled { |
There was a problem hiding this comment.
made these changes in a non-breaking way. IMO the current manifestation of the WorkflowDebugger API has not seemed to provide a ton of value for us internally. it might be worth considering removing it in the next major version bump, which would allow some of this code to be cleaned up.
| func toDebugInfoSource() -> WorkflowUpdateDebugInfo.Source { | ||
| switch self { | ||
| case .external: .external | ||
| case .subtree(let maybeInfo): .subtree(maybeInfo.unwrappedOrErrorDefault) | ||
| } |
There was a problem hiding this comment.
the other two cases of debug info source (worker and sideeffect) seemed to never have been provided here AFAICT, so we only bother representing the other two cases in the new type.
| extension WorkflowNode.SubtreeManager { | ||
| enum Output { | ||
| case update(any WorkflowAction<WorkflowType>, source: WorkflowUpdateDebugInfo.Source) | ||
| case childDidUpdate(WorkflowUpdateDebugInfo) | ||
| case update(any WorkflowAction<WorkflowType>, source: EventSource) |
There was a problem hiding this comment.
changed this from using the public type to a new internal one
| } | ||
|
|
||
| extension WorkflowUpdateDebugInfo { | ||
| fileprivate static let unexpectedlyMissing = { |
There was a problem hiding this comment.
in a few places we want to propagate debug info with the 'right' optionality. either it should be nil if there's no debugger, or non-nil if there is. at the root level we need to emit a value to the debugger if present that is non-optional, hence this 'sanity check' value that will hopefully ensure we don't mess things up.
There was a problem hiding this comment.
Is there an integration test we could write to validate this behavior, or some other mechanism to solidify this contract?
There was a problem hiding this comment.
i added a few more unit tests to cover the 'set a debugger' and 'didn't set a debugger' cases
|
|
||
| func test_conditional_debug_info_with_debugger() { | ||
| let subject = HostContext.testing(debugger: TestDebugger()) | ||
| let expectaiton = expectation(description: "debugger block invoked") |
There was a problem hiding this comment.
| let expectaiton = expectation(description: "debugger block invoked") | |
| let expectation = expectation(description: "debugger block invoked") |
Did you know you can turn on spellcheck in Xcode?
There was a problem hiding this comment.
ha... yes i do, and (perhaps surprisingly) do in fact have that setting enabled. guess i was not paying very close attention...
it's largely theoretical, and i'd expect any 'real world' gains to be small and amortized, but the code we're currently always running does call runtime functions to produce strings from types, which is not the cheapest thing ever. i did a rough sanity check locally when applying an action through a nested chain of workflows 1000 nodes long, and it appeared consistently faster to skip the debug info creation if not needed. |
refactors action propagation to no longer instantiate any
WorkflowDebugInfotypes unless there is aWorkflowDebuggerset on the host. this should avoid some unnecessary string allocations and reflection metadata lookups.Checklist