-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Allow use of local buffers #170
Conversation
src/utils.jl
Outdated
function lockbuffer!(f, params) | ||
if params.threaded | ||
lock(params.buffer_lock) do | ||
return f() | ||
end | ||
else | ||
return f() | ||
end | ||
end | ||
function lockmergesbuffer!(f, params) | ||
if params.threaded | ||
lock(params.merges_buffer_lock) do | ||
return f() | ||
end | ||
else | ||
return f() | ||
end | ||
end | ||
|
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.
Why lock the buffer only if threaded?
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.
Do we need to lock if not threaded? The docs are fairly clear that you shouldn't take locks in generated functions (although this is likely to be problematic mostly for globals I was told it's probably best to avoid them even when passed in).
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 switched the name of this parameter in the EGraph to needslock
. We don't need to lock within a generated function, so I'd like to keep that functionality. But that separates it out from threading.
It's also not clear to me what params.threaded
does? It doesn't seem to be used anywhere.
Thanks a lot :) |
Thanks Raye! |
@0x0f0f0f I am a little confused about the actual uses of the locks. Does anything internal actually access the global buffers simultaneously? Or is it just to support user threading? The only threading appears to be in Rewriters. If it's just users then probably |
e-matching search can be done in parallel since it doesn't mutate the e-graph. it was parallelized in previous versions and can be reintroduced.
|
Okay so can we keep Once it is we can be sure threading is disabled if We can't thread within generated functions anyway, so it would be disabled then and we would want to avoid locking. Perhaps |
Rereading, I wonder if I've misunderstood the purpose of the buffer locks. It seems that they do more than just lock buffers for push or pop, there are several statements and even entire functions inside the locked regions. Do the locks need to guard so much code, or is it possible to move the locks to surround just the buffers? If the locks need to guard such large regions of code, perhaps concurrent collections will not become useful. |
I was originally suggesting concurrent collections as it would allow an interface where
would be roughly equivalent to something like
So that the queues could be responsible for their own locks. Then, if you wanted to thread the code you would use a thread-safe buffer. If you wanted to run serial you would use a regular buffer. then you could select |
so long as each parallel phase of the algorithm either only pushes or only pops, I think we can move the locks to surround just the buffers and perhaps even make the code more parallel. Is there a phase of the algorithm where one thread might push to a buffer while another thread pops or reads from that same buffer? |
Ah, I see. I think that Metatheory.jl/src/EGraphs/egraph.jl Line 372 in 7828344
Metatheory.jl/src/EGraphs/saturation.jl Lines 247 to 252 in 7828344
In this instance, |
@0x0f0f0f Why does every egraph need to share the same buffer? This adds complexity, and I'm not sure what the benefits are: Pros:
Cons:
Could we just make all buffers local? This wouldn't preclude egraph threading later, and you could still reuse the buffers by passing the same buffers into multiple egraphs. |
Yes, would default to
That's alright.
Exactly, but |
@rayegun I'll make a new branch and try just using vectors for buffers. Let's compare the performance. |
Performance example with vector buffers:
|
Compared to concurrent deques:
|
This is a basic attempt to address #156. It changes nothing about the default behavior, but allows (generated function) local buffers to be supplied to
saturate!
. To save space it may be possible (if frowned upon) to use thread local or scoped buffers within a generated function, but I did not test this (future compiler threading could make that impossible).Basic benchmarking indicates no performance impact, but an expert should weigh in here.
I didn't use ConcurrentCollections because spawning tasks inside of a generated function is questionable anyway, so threading should be disabled there.