-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
alternative to #18185 #18206
alternative to #18185 #18206
Conversation
when false: | ||
# make rlocks modlue consistent with locks module, | ||
# so they can replace each other seamlessly. | ||
{.error: "Rlocks requires --threads:on option.".} |
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.
nit: pre-existing but {.error: "rlocks requires --threads:on option.".}
(lowercase r) would make more sense
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.
It is consistent with pre-existing marks
"ThreadPool"
"Locks"
...
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.
there is not Rlocks
object name though... it's the whole module that (before this PR) was supposed to be disabled; anyway it's now disabled, and it's a nit
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.
LGTM for following reasons:
if we start requiring threads:on for sharedtables, it's a rabbithole, other modules would then also require threads:on
even if they don't need to (refs #18185 (comment))
threads:on can have negative performance implications when a single thread is needed, as well cause other issues eg around gcsafety for otherwise working code.
sharedtables may be designed for threads:on but it doens't mean it should require it, and in fact nim r --threads:off tests/stdlib/tsharedtable.nim
has been working again since #12331.
rlocks (like locks) should indeed work with threads:off, so that apps can support threads:on|off without having to patch a lot of code with things like when compileOption("threads"): ...
I'm not sure what linker error was observed (refs #12231 (comment), no details were provided), but if there's a repro we can always find a solution to avoid the link error that doesn't involve erroring for import rlocks
for threads:off
close #18185
Pros
The rlocks module is more consistent with locks module, so they can replace each other seamlessly.
Cons
The error messages may be worse as #12231 said(link error instead of compilation error without threads options)
Other possible solution:
make
threads:on
options default may solve this problem(ref #10781 and nim-lang/RFCs#361)