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

@lock export from Base, closes #36441 #39588

Merged
merged 8 commits into from
Apr 30, 2021
Merged

Conversation

sairus7
Copy link
Contributor

@sairus7 sairus7 commented Feb 9, 2021

No description provided.

@ronisbr
Copy link
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
Member

vtjnash commented Apr 22, 2021

bump? needs docs and news for this

@sairus7
Copy link
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
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
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
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
Contributor Author

sairus7 commented Apr 28, 2021

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

@ronisbr
Copy link
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
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.

base/lock.jl Outdated Show resolved Hide resolved
base/exports.jl Outdated Show resolved Hide resolved
@sairus7
Copy link
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
Member

NHDaly commented Apr 30, 2021

Fantastic, thanks all! :) 🙌

@ronisbr
Copy link
Member

ronisbr commented Apr 30, 2021

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

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