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

Can we export, document, and emphasize the at-lock macro? #36441

Closed
NHDaly opened this issue Jun 26, 2020 · 2 comments
Closed

Can we export, document, and emphasize the at-lock macro? #36441

NHDaly opened this issue Jun 26, 2020 · 2 comments
Labels
docs This change adds or pertains to documentation feature Indicates new feature / enhancement requests good first issue Indicates a good issue for first-time contributors to Julia multithreading Base.Threads and related functionality

Comments

@NHDaly
Copy link
Member

NHDaly commented Jun 26, 2020

For more context: https://discourse.julialang.org/t/poor-performance-of-lock-l-do-closures-caused-by-poor-performance-of-capturing-closures/42067/13

Base already provides a @lock l expr macro, which (as outlined in the above discourse thread) is often more performant than the related function form lock(f, l).

julia/base/lock.jl

Lines 178 to 188 in 6c9bc51

macro lock(l, expr)
quote
temp = $(esc(l))
lock(temp)
try
$(esc(expr))
finally
unlock(temp)
end
end
end

But this macro is not very discoverable, for at least these reasons, and is not widely used:

  1. It's not currently exported, so not very discoverable:
    julia> @lock ReentrantLock() 2
    ERROR: LoadError: UndefVarError: @lock not defined
    in expression starting at REPL[1]:1
  2. It's not mentioned in any of the docstrings for locks, so again not very discoverable
  3. It doesn't have its own docstring, so not very discoverable
  4. It seems like this should be preferred over the existing lock(f, lock). Can we deprecate that in favor of this?

Can we export it, add docstrings for it, and maybe reference it from the other docstrings?

And/or maybe even emphasize that you should probably prefer this over the function form?

@NHDaly
Copy link
Member Author

NHDaly commented Jun 26, 2020

This is all coming from us spending a few days digging into poor performance in some central pieces of our codebase, only to find we were using the function form of lock(f, l) all over the place, with nontrivial cost! :'(

@NHDaly NHDaly added docs This change adds or pertains to documentation feature Indicates new feature / enhancement requests multithreading Base.Threads and related functionality good first issue Indicates a good issue for first-time contributors to Julia labels Dec 20, 2020
sairus7 added a commit to sairus7/julia that referenced this issue Feb 9, 2021
sairus7 added a commit to sairus7/julia that referenced this issue Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue May 4, 2021
jarlebring pushed a commit to jarlebring/julia that referenced this issue May 4, 2021
jarlebring pushed a commit to jarlebring/julia that referenced this issue May 6, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this issue May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this issue Jul 5, 2021
@xgdgsc
Copy link
Contributor

xgdgsc commented Jul 10, 2023

So this is still not documented?

@xgdgsc xgdgsc mentioned this issue Jul 10, 2023
ViralBShah pushed a commit that referenced this issue Aug 25, 2023
omus added a commit that referenced this issue Jan 2, 2024
Follow up to: #36441. Makes
this macro much easier to learn about if you're just viewing the
docstrings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation feature Indicates new feature / enhancement requests good first issue Indicates a good issue for first-time contributors to Julia multithreading Base.Threads and related functionality
Projects
None yet
Development

No branches or pull requests

2 participants