Skip to content

@lock export from Base, closes #36441#39588

Merged
vtjnash merged 8 commits intoJuliaLang:masterfrom
sairus7:patch-1
Apr 30, 2021
Merged

@lock export from Base, closes #36441#39588
vtjnash merged 8 commits intoJuliaLang:masterfrom
sairus7:patch-1

Conversation

@sairus7
Copy link
Copy Markdown
Contributor

@sairus7 sairus7 commented Feb 9, 2021

No description provided.

@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Feb 11, 2021

Hi @sairus7

Thanks for this PR! IMHO, to truly close #36441 we need to do the following in addition to export @lock:

  1. Add a docstring to this macro.
  2. Add a small text in the documentation showing in which contexts it should be used.

For more information, you can see: https://discourse.julialang.org/t/poor-performance-of-lock-l-do-closures-caused-by-poor-performance-of-capturing-closures/42067/13

EDIT: BTW, we also have the macro @lock_nofail for cases in which we can guarantee that the function will not throw any error. In this case, avoiding try catch can improve the performance. Hence, maybe we can also export this macro and add the documentation about it in this PR.

@ronisbr ronisbr added the multithreading Base.Threads and related functionality label Feb 11, 2021
@JeffBezanson JeffBezanson added needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Feb 12, 2021
@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Apr 22, 2021

bump? needs docs and news for this

@sairus7
Copy link
Copy Markdown
Contributor Author

sairus7 commented Apr 28, 2021

@ronisbr I've added docstrings and @lock_nofail export.
Does it cover 1 and 2, or should I seach for some other places in documentation to add a mention about these macro? If so, can you point me to it?

@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Apr 28, 2021

Hi @sairus7 !

For me it is perfect and fully addressed my points 1 and 2. I think we only need to add an entry in NEWS.md informing that @lock and @lock_nofail is now exported.

@ronisbr ronisbr removed the needs docs Documentation for this change is required label Apr 28, 2021
@sairus7
Copy link
Copy Markdown
Contributor Author

sairus7 commented Apr 28, 2021

add an entry in NEWS.md informing that @lock and @lock_nofail is now exported

Seems NEWS.md file is excluded from julia repo, so how to do this?

@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Apr 28, 2021

Seems NEWS.md file is excluded from julia repo, so how to do this?

No, it does not seem to be excluded, check here:

https://github.com/JuliaLang/julia/blob/master/NEWS.md

@sairus7
Copy link
Copy Markdown
Contributor Author

sairus7 commented Apr 28, 2021

Added this to news, not sure if the right section.

@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Apr 28, 2021

LGTM!

@ronisbr ronisbr removed the needs news A NEWS entry is required for this change label Apr 28, 2021
@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Apr 28, 2021

@sairus7

We have a problem in the building:

base/lock.jl:208:`lock(l)` and `unlock(l)` functions. It is often more performant than function form 
base/lock.jl:227:Equivalent to `@lock l expr` for cases in which we can guarantee that the function 
Error: trailing whitespace found in source file(s)

I believe you need to remove trailing spaces from those doc strings.

@sairus7
Copy link
Copy Markdown
Contributor Author

sairus7 commented Apr 30, 2021

I think all conversations above are resolved now.

@fredrikekre fredrikekre requested a review from vtjnash April 30, 2021 09:28
@vtjnash vtjnash merged commit d7c6a9b into JuliaLang:master Apr 30, 2021
@NHDaly
Copy link
Copy Markdown
Member

NHDaly commented Apr 30, 2021

Fantastic, thanks all! :) 🙌

@ronisbr
Copy link
Copy Markdown
Member

ronisbr commented Apr 30, 2021

Nice!! Thank you @sairus7 for you contribution :)

@sairus7 sairus7 deleted the patch-1 branch April 30, 2021 16:36
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
jarlebring pushed a commit to jarlebring/julia that referenced this pull request May 4, 2021
jarlebring pushed a commit to jarlebring/julia that referenced this pull request May 6, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
giordano added a commit that referenced this pull request Nov 29, 2025
It was exported in v1.7, not v1.10: #39588
Keno pushed a commit that referenced this pull request Nov 29, 2025
KristofferC pushed a commit that referenced this pull request Dec 4, 2025
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
KristofferC pushed a commit that referenced this pull request Dec 4, 2025
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
KristofferC pushed a commit that referenced this pull request Dec 16, 2025
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
KristofferC pushed a commit that referenced this pull request Dec 16, 2025
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
DilumAluthge pushed a commit that referenced this pull request Jan 14, 2026
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
DilumAluthge pushed a commit that referenced this pull request Jan 19, 2026
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
DilumAluthge pushed a commit that referenced this pull request Jan 20, 2026
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
DilumAluthge pushed a commit that referenced this pull request Jan 29, 2026
It was exported in v1.7, not v1.10: #39588

(cherry picked from commit 1067db8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

multithreading Base.Threads and related functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants