-
Notifications
You must be signed in to change notification settings - Fork 698
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
Pass Context at runtime not compile time #1904
Comments
@gcatron Thanks for doing this work. I have a question about the migration plan. Maybe I missed somethings. Would it make sense to start the process by adding the ctx parameter to run? I think that if you start by moving the ctx parameter to run you won't need to add the new executeCtx and compileCtx APIs. |
We probably could get away without compileCtx for sure. My reasoning for adding executeCtx was to maintain an unbroken execution flow. I think the runtime allocation logic will need to reside in the execute method implementations as opposed to run. (would be happy to be wrong on that point) |
I think the end state is the right one. But I'd second @nadavrot on avoiding adding the *Ctx versions if it's possible. Since there are a ton of tests to port anyways, I worry that you'll find yourself spending a ton of time swizzling names :-p. (Sometimes you can do these migrations quickly with |
Documenting a conversation @opti-mix and I just had. A candidate API exposed by the CompiledFunction, setupRuns would be run once and setup everything that can be done for all runs. |
In order to facilitate multi-threaded execution context passing needs to be moved from compile time to runtime. This will break the current API!
At compile time, constant weight allocation will remain unchanged, however Placeholder memory will no longer be allocated at runtime. The expected sizes will be calculated from the the Placeholder types.
At runtime (ExecutionEngine::run) the context will be passed in and Placeholder memory will be assigned to point to the allocations in the context.
When done context will no longer be a parameter of the compile function. ExecutionEngine::run and function::execute will expect a context parameter.
This change will be done in stages to minimize disruption.
ExecutionEngine::run(Context &ctx) will be introduced. This is the new execution flow. When the bugs are worked out it will replace run. Tests will be ported to use run(ctx).
CompiledFunction::execute(Context &ctx) will be introduced, similar to run(ctx) this will replace execute() when the work is finished. Tests will be ported to execute(Context &ctx).
execute(Context &ctx) will be updated to handle memory allocation at runtime.
Once runctx and execute(ctx) are working and all tests pass run(ctx) will replace run and execute(ctx) will replace execute. At this point any external code will need to be updated to reflect these changes.
The text was updated successfully, but these errors were encountered: