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

Do not migrate tasks to a different thread pool (backport of #1159) #1206

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

Drvi
Copy link
Collaborator

@Drvi Drvi commented Dec 12, 2024

No description provided.

@Drvi Drvi requested review from quinnj and NHDaly December 12, 2024 13:24
@IanButterworth
Copy link
Member

This seems like functionality Base should have. Is there a feature request for it?

@Drvi
Copy link
Collaborator Author

Drvi commented Dec 12, 2024

@IanButterworth I'm not aware, but I think that would be Threads.@spawn Threads.threadpool() ....

@Drvi
Copy link
Collaborator Author

Drvi commented Dec 12, 2024

I think the tests are broken on 1.11+ because we're touching internals that changed with the Memory PR.

@IanButterworth
Copy link
Member

Threads.@spawn Threads.threadpool()

That looks good to me. Any reason not to use that?

@Drvi
Copy link
Collaborator Author

Drvi commented Dec 12, 2024

Any reason not to use that?

The macro from ConcurrentUtilities is compatible with older Julia versions:

macro samethreadpool_spawn(expr)
    if VERSION >= v"1.9.2"
        esc(:(Threads.@spawn Threads.threadpool() $expr))
    else
        esc(:(Threads.@spawn $expr))
    end
end

Since we depend on the packege already and given the original PR also went this route, I simply did the same.

Copy link
Collaborator

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me 👍

@Drvi Drvi merged commit 0cfe5ce into release-1.9 Dec 13, 2024
3 of 11 checks passed
@Drvi Drvi deleted the td-backport-1159 branch December 13, 2024 11:49
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