-
Notifications
You must be signed in to change notification settings - Fork 10
perf: memoize IR plan calls to reduce reevaluation cost #36
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
Changes from all commits
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,60 @@ | ||
| import AST | ||
| import Foundation | ||
| import IR | ||
|
|
||
| /// InvocationKey is a key for memoizing an IR function call invocation. | ||
| /// Note we capture the arguments as unresolved operands and not resolved values, | ||
| /// as hashing the values was proving extremely expensive. We instead rely on the | ||
| /// invariant the the plan / evaluator will not modify a local after it has been initally set. | ||
| struct InvocationKey: Hashable { | ||
| let funcName: String | ||
| let args: [IR.Operand] | ||
| } | ||
|
|
||
| /// MemoCache is a memoization cache of plan invocations | ||
| typealias MemoCache = [InvocationKey: AST.RegoValue] | ||
|
|
||
| /// MemoStack is a stack of MemoCaches | ||
| final class MemoStack { | ||
| var stack: [MemoCache] = [] | ||
| } | ||
|
|
||
| extension MemoStack { | ||
| /// Get and set values on the cache at the top of the memo stack. | ||
| subscript(key: InvocationKey) -> AST.RegoValue? { | ||
| get { | ||
| guard !self.stack.isEmpty else { | ||
| return nil | ||
| } | ||
| return self.stack[self.stack.count - 1][key] | ||
| } | ||
| set { | ||
| if self.stack.isEmpty { | ||
| self.stack.append(MemoCache.init()) | ||
| } | ||
| self.stack[self.stack.count - 1][key] = newValue | ||
| } | ||
| } | ||
|
|
||
| func push() { | ||
|
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. Suggestion: Mark these funcs as
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. Thanks! I suggest we revisit this optimization if we see any measurable impact - my hunch is that we will not see excessive |
||
| self.stack.append(MemoCache.init()) | ||
| } | ||
|
|
||
| func pop() { | ||
| guard !self.stack.isEmpty else { | ||
| return | ||
| } | ||
| self.stack.removeLast() | ||
| } | ||
|
|
||
| /// withPush returns the result of calling the provided closure with | ||
| /// a fresh memoCache pushed on the stack. The memoCache will only be | ||
| /// active during that call, and discarded when it completes. | ||
| func withPush<T>(_ body: () async throws -> T) async rethrows -> T { | ||
|
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. You could use
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. Also, probably a good idea to explicitly pass down the isolation domain via:
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. It's not clear to me that those are needed here? We're not doing an actor hop that I can see, and this stack is only used within a single evaluation, not across evaluations. Please let me know if I'm misunderstanding 🤔 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. Any method without isolation inheritance is going to hop defensively. The recommendation with Swift 6.2 is to adopt the upcoming language feature
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. @shomron That shouldn't be a huge performance loss in this case, however this applies to the entirety of |
||
| self.push() | ||
| defer { | ||
| self.pop() | ||
| } | ||
| return try await body() | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Sweet, I like the
with-style syntax in this use case, great idea that prohibits forgetting to pop.