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

Rule for making @threads not recommended #1

Closed
wants to merge 4 commits into from

Conversation

iuliadmtru
Copy link
Collaborator

@iuliadmtru iuliadmtru commented Jul 6, 2023

According to this blogpost, Threads.@threads should not be used without :static, or at all, preferring working directly with tasks using Threads.@spawn. I added a rule that searches for uses of Threads.@threads, but not for Threads.@threads :static.

When running semgrep -c on the .yml and .jl files, there are three matches:

threads.threads-macro                                                         
          Using `Threads.@threads` can possibly lead to concurrency bugs. Use `Threads.@threads
          :static` instead or work directly with `Threads.@spawn` to create and manage tasks.  
                                                                                               
            6┆ Threads.@threads for i = 1:10
            7┆     a[i] = Threads.threadid()
            ⋮┆----------------------------------------
            6┆ Threads.@threads for i = 1:10
            7┆     a[i] = Threads.threadid()
            ⋮┆----------------------------------------
           11┆ @threads for i = 1:10
           12┆     a[i] = threadid()

Lines 6-7 are matched two times. This is because of the pattern-either rule. This rule would not be necessary if Semgrep would support matching wildcard imports (such as using Threads), making @threads be recognisable as Threads.@threads and removing the need for checking both patterns. I understand from @brandonspark that they are working on making this possible.

Also, I don't understand why the end line won't match as well (why, for example, doesn't the rule match lines 6-8 instead of just 6-7). But I don't think that's a big problem.

@iuliadmtru iuliadmtru changed the title Rule for Threads.@threads Rule for making @threads not recommended Jul 6, 2023
@KristofferC
Copy link

According to this blogpost, Threads.@threads should not be used without :static, or at all, preferring working directly with tasks using Threads.@Spawn.

That seems a bit strong. As long as you don't have a "work cache" that you try to index with the thread id it should be fine. The default for @threads was changed from :static to :dynamic since :dynamic has in general better performance since you are not a priori deciding what threads will work on different chunks of the data.

@iuliadmtru
Copy link
Collaborator Author

I must have misunderstood the discussion. I think the other PR I did is what you mean?

Also, do I understand correctly that nthreads() should stop being used?

@aviks aviks closed this Jul 13, 2023
@aviks
Copy link
Member

aviks commented Jul 13, 2023

While there is some discussion that @threads should be deprecated in favour of something else, that doesn't seem to be consensus opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants