Skip to content

avoid Core.Box in the package#161

Merged
DilumAluthge merged 2 commits into
JuliaLang:masterfrom
KristofferC:kc/boxes
Feb 20, 2026
Merged

avoid Core.Box in the package#161
DilumAluthge merged 2 commits into
JuliaLang:masterfrom
KristofferC:kc/boxes

Conversation

@KristofferC

Copy link
Copy Markdown
Member

Fixes the Distributed part of JuliaLang/julia#60479, namely:

var func sig location
x #remotecall_pool#130 Tuple{Distributed.var"##remotecall_pool#130", Base.Pairs{Symbol, V, Nothing, NT} where {V, NT<:NamedTuple}, typeof(Distributed.remotecall_pool), typeof(Distributed.remotecall), Any, Distributed.AbstractWorkerPool, Vararg{Any}} /home/kc/julia/usr/share/julia/stdlib/v1.14/Distributed/src/workerpool.jl:140
x #remotecall_pool#154 Tuple{Distributed.var"##remotecall_pool#154", Base.Pairs{Symbol, V, Nothing, NT} where {V, NT<:NamedTuple}, typeof(Distributed.remotecall_pool), typeof(Distributed.remotecall), Any, Distributed.CachingPool, Vararg{Any}} /home/kc/julia/usr/share/julia/stdlib/v1.14/Distributed/src/workerpool.jl:407
sock #start_worker#24 Tuple{Distributed.var"##start_worker#24", Bool, Bool, typeof(Distributed.start_worker), IO, AbstractString} /home/kc/julia/usr/share/julia/stdlib/v1.14/Distributed/src/cluster.jl:237
w create_worker Tuple{typeof(Distributed.create_worker), Any, Any} /home/kc/julia/usr/share/julia/stdlib/v1.14/Distributed/src/cluster.jl:601

I couldn't run the tests locally because I immidiately hit:

Test Failed at /home/kc/JuliaPkgs/Distributed.jl/test/distributed_exec.jl:10
  Expression: startswith(pathof(Distributed), sharedir)
   Evaluated: startswith("/home/kc/JuliaPkgs/Distributed.jl/src/Distributed.jl", "/home/kc/.julia/juliaup/julia-nightly/share")
ERROR: LoadError: There was an error during testing
in expression starting at /home/kc/JuliaPkgs/Distributed.jl/test/distributed_exec.jl:7
Test Failed at /home/kc/JuliaPkgs/Distributed.jl/test/runtests.jl:19
  Expression: success(pipeline(cmd; stdout = stdout, stderr = stderr)) && ccall(:jl_running_on_valgrind, Cint, ()) == 0
     Context: cmd = `/home/kc/.julia/juliaup/julia-nightly/bin/julia -C native -J/home/kc/.julia/juliaup/julia-nightly/lib/julia/sys.so -g1 --color=yes --startup-file=no --check-bounds=yes --startup-file=no --depwarn=error /home/kc/JuliaPkgs/Distributed.jl/test/distributed_exec.jl`
ERROR: LoadError: There was an error during testing
in expression starting at /home/kc/JuliaPkgs/Distributed.jl/test/runtests.jl:18
ERROR: Package Distributed errored during testing

@codecov

codecov Bot commented Jan 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.41%. Comparing base (a6195c0) to head (9762351).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
+ Coverage   79.34%   79.41%   +0.07%     
==========================================
  Files          10       10              
  Lines        1951     1953       +2     
==========================================
+ Hits         1548     1551       +3     
+ Misses        403      402       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KristofferC

Copy link
Copy Markdown
Member Author

cc @JamesWrigley, @MilesCranmer

Comment thread src/cluster.jl
Comment on lines 250 to +252
(port, sock) = listenany(interface, UInt16(port_hint))
LPROC.bind_port = port
sock

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
(port, sock) = listenany(interface, UInt16(port_hint))
LPROC.bind_port = port
sock
let (port, sock) = listenany(interface, UInt16(port_hint))
LPROC.bind_port = port
sock
end

but might not be needed

Comment thread src/cluster.jl Outdated
Comment on lines 256 to 261
let sock_local = sock
errormonitor(@async while isopen(sock_local)
client = accept(sock_local)
process_messages(client, client, true)
end)
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let sock_local = sock
errormonitor(@async while isopen(sock_local)
client = accept(sock_local)
process_messages(client, client, true)
end)
end
let sock = sock
errormonitor(@async while isopen(sock)
client = accept(sock)
process_messages(client, client, true)
end)
end

just for clarity, since it is the same underlying object ultimately

Comment thread src/cluster.jl Outdated
Comment on lines 612 to 621
try
(r_s, w_s) = connect(manager, w.id, wconfig)
(r_s, w_s) = connect(manager, w_stub.id, wconfig)
catch ex
try
deregister_worker(w.id)
kill(manager, w.id, wconfig)
deregister_worker(w_stub.id)
kill(manager, w_stub.id, wconfig)
finally
rethrow(ex)
end
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
try
(r_s, w_s) = connect(manager, w.id, wconfig)
(r_s, w_s) = connect(manager, w_stub.id, wconfig)
catch ex
try
deregister_worker(w.id)
kill(manager, w.id, wconfig)
deregister_worker(w_stub.id)
kill(manager, w_stub.id, wconfig)
finally
rethrow(ex)
end
end
r_s, w_s = try
connect(manager, w_stub.id, wconfig)
catch ex
try
deregister_worker(w_stub.id)
kill(manager, w_stub.id, wconfig)
finally
rethrow(ex)
end
end

Comment thread src/cluster.jl Outdated
# initiate a connect. Does not wait for connection completion in case of TCP.
w = Worker()
w_stub = Worker()
local r_s, w_s

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
local r_s, w_s

@KristofferC

Copy link
Copy Markdown
Member Author

Updated

@KristofferC

Copy link
Copy Markdown
Member Author

bump

Comment thread src/cluster.jl
local r_s, w_s
try
(r_s, w_s) = connect(manager, w.id, wconfig)
w_stub = Worker()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For this and the socket case above, would setting the type of the variable also fix the boxing? e.g.:

Suggested change
w_stub = Worker()
w::Worker = Worker()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity, why not?

BTW, in almost every interaction I've had with people giving one word replies (which is quite a lot in the Julia community for some reason) I end up having to ask more clarifying questions and the whole process takes much more time than it would have taken to have written a proper answer in the first place. Spending less time communicating is not more efficient 😅

@KristofferC

Copy link
Copy Markdown
Member Author

At least 1 approving review is required by reviewers with write access.

This is such a useless thing to enable on a repo.

@JamesWrigley

Copy link
Copy Markdown
Member

At least 1 approving review is required by reviewers with write access.

This is such a useless thing to enable on a repo.

This has also annoyed me a lot, can you disable it?

@JamesWrigley JamesWrigley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@DilumAluthge

Copy link
Copy Markdown
Member

At least 1 approving review is required by reviewers with write access.

This is such a useless thing to enable on a repo.

This has also annoyed me a lot, can you disable it?

I've disabled that setting

@DilumAluthge

Copy link
Copy Markdown
Member

I see that this was approved, but not merged.

@JamesWrigley Can I merge this PR? Or is there more reviewing to do?

@JamesWrigley

Copy link
Copy Markdown
Member

I think you can merge it 👍

@DilumAluthge DilumAluthge merged commit 3ebddd3 into JuliaLang:master Feb 20, 2026
8 checks passed
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.

4 participants