Skip to content
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

Closed
gcatron opened this issue Oct 22, 2018 · 4 comments
Closed

Pass Context at runtime not compile time #1904

gcatron opened this issue Oct 22, 2018 · 4 comments

Comments

@gcatron
Copy link
Contributor

gcatron commented Oct 22, 2018

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.

  1. 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).

  2. 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).

  3. execute(Context &ctx) will be updated to handle memory allocation at runtime.

  4. 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.

@nadavrot
Copy link
Contributor

@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.

@gcatron
Copy link
Contributor Author

gcatron commented Oct 22, 2018

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)

@bertmaher
Copy link
Contributor

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 sed but if not it's a lot of manual pain).

@gcatron
Copy link
Contributor Author

gcatron commented Nov 7, 2018

Documenting a conversation @opti-mix and I just had. A candidate API exposed by the CompiledFunction,
Similar to JUnit beforeClass afterClass before after:
setupRuns x1
beforeRun, afterRun xN
tearDownRuns x1

setupRuns would be run once and setup everything that can be done for all runs.
before/afterRun would be what is needed on a per run basis.
tearDownRuns would cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants