-
Notifications
You must be signed in to change notification settings - Fork 321
Add spawn/join concurrency extension to Bril #416
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
base: main
Are you sure you want to change the base?
Conversation
sampsyo
left a comment
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.
Awesome; really nice work on this language extension and its implementation here! This is an excellent start.
Aside from some code-level suggestions, I have a few cross-cutting thoughts:
- The stuff in
benchmarksis (mostly?) better thought of as correctness tests. Benchmarks are more real-ish algorithms. So with the possible exception of the dot product program, let's move these to thetest/interpandtest/interp-errordirectories. - A main theme in my comments about the interpreter extension is that it would be possible to avoid some global state, which will keep the interpreter more maintainable/composable. Let me know if this stuff makes sense.
Strategically, I guess I want to suggest that we maybe start by reducing the scope of this PR, get it merged, and defer some extensions to subsequent steps. Would it be possible to trim this PR down to just these things:
- the docs
- the worker-free sequential fallback implementation in the interpreter
- the tests
If we leave out the "real parallelism" implementation and the escape analysis, that should make this PR easy to finalize, and then we can address those things separately. Would that make sense to you, @ananyagoenka?
| "store" | "free" | | ||
| "speculate" | "guard" | "commit"; | ||
| "store" | "free" | | ||
| "speculate" | "guard" | "commit" | "join"; |
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.
I'm not sure why the indentation changed here (differences in formatter configuration?), but it would be great to change it back.
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.
I have generally tried to separate what goes in bril-ts, which is a general-purpose TypeScript library for interacting with Bril programs, from the interpreter that goes in brili.ts. Since this is pretty tightly coupled with the interpreter, I think we probably need to create a separate brili directory where this can live alongside the main source file.
| private next = 1; | ||
| private wait = new Map<number, (v: any) => void>(); |
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.
I think this machinery could use some comments—it's an interesting approach that involves message-ID-keyed callbacks that is kinda subtle. Maybe a high-level overview of the technique in English could help demystify this a bit?
| return new Promise((resolve) => { | ||
| this.wait.set(id, resolve); | ||
| // send request back to main thread | ||
| (self as DedicatedWorkerGlobalScope).postMessage({ |
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.
Sorry if this is a silly question, but why is it safe to assert self as DedicatedWorkerGlobalScope? I thought that self here would be a HeapProxy, but self is tricky in JavaScript…
| const sharedHeap = new HeapProxy<any>(); | ||
|
|
||
| // Message handler | ||
| self.onmessage = async (ev: MessageEvent) => { |
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.
I'm a little confused about what self is here—aren't we at the top level of a source file?
| * Creates a new thread that begins executing function @func. | ||
| * The caller’s current values of arg1 … argN are copied into the callee’s fresh environment; subsequent changes are not shared. | ||
| * The thread shares the global heap with every other thread in the | ||
| program. | ||
| * Returns a fresh thread handle that uniquely identifies the spawned | ||
| thread. |
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.
Let's just make this a plain old paragraph instead of a bulleted list (i.e., please remove the bullets).
| "args": ["arg1", "arg2", "..."] | ||
| } | ||
|
|
||
| * Creates a new thread that begins executing function @func. |
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.
Markdown backticks would be helpful here, and in other places where you refer to parts of the instruction:
| * Creates a new thread that begins executing function @func. | |
| * Creates a new thread that begins executing function `@func`. |
| * spawn must provide exactly one function name in funcs. | ||
| * The number of args to spawn must equal the callee’s formal-parameter list. | ||
| * The destination of spawn must be declared with type thread. | ||
| * The sole argument to join must have type thread. |
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.
Please move these requirements to the sections that describe each of the relevant instructions.
| 1 | ||
| 2 | ||
| 0 No newline at end of file |
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.
Please mark this as code so it gets laid out correctly.
| * Heap allocations created with alloc are global; every thread | ||
| may load or store through any pointer value it holds. | ||
| * If two threads access the same location and at least one access is a | ||
| store, the program has a data race, and the result is undefined. |
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.
Maybe it would be nice to link to the SSA and memory extension documentation pages here.
This PR implements a lightweight thread‐based concurrency extension for Bril (closes #499).
Highlights
Specification
docs/extensions/concurrency.md: describes the newthreadtype,spawn(value‐op) andjoin(effect‐op), JSON syntax, and well‐formedness rules.docs/syntax.md: updated to listspawnandjoin, and thethreadprimitive.Parser Updates
tools/text/briltxt.py: Lark grammar now recognizesspawnandjoin.TypeScript Definitions
bril-ts/bril.ts:PrimTypeincludes"thread".ValueOpCodeadds"spawn",EffectOpCodeadds"join".typeCheckandargCountsupdated.tests/concurrency_types_test.ts: simpledeno checksmoke test for new ops.Interpreter Runtime
src/brili.ts:ThreadHandle, a globalthreadTable, andnextTid.spawn(copies args into a freshEnv, launches an async task, returns handle) andjoin(awaits the associated promise).--no-workersflag for in-process async mode.Concurrency Test Suite
benchmarks/concurrency/:*.bril+*.out) covering correct behavior and errors.turnt.toml: addsbrili-concenv to run the suite underdeno run -A brili.ts.Micro-benchmarks & Performance
benchmarks/concurrency/perf/:perf_par_sum_100k.bril,perf_matmul_split.bril,perf_big_par_sum_10M.brilrun_perf.shmeasures sequential vs concurrent speed-up.Escape Analysis Pass
examples/escape.py: intraprocedural pass that reports thread-localallocs.tools/README.md: how to runescape.pyviabril2json | python3 | deno run.