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

support foreign threads #46609

Merged
merged 2 commits into from
Oct 14, 2022
Merged

support foreign threads #46609

merged 2 commits into from
Oct 14, 2022

Conversation

JeffBezanson
Copy link
Sponsor Member

@JeffBezanson JeffBezanson commented Sep 2, 2022

Rebased #45447 and moved to this repo.

Fixes #17573

@JeffBezanson JeffBezanson added the multithreading Base.Threads and related functionality label Sep 2, 2022
@IanButterworth
Copy link
Sponsor Member

It would be good to fix (remove) the compat note in the docstring for Profile.init in the first commit here, as that commit will I assume be backported to 1.8

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 7, 2022

This assertion seems odd:

      From worker 2:	Assertion failed: gc_num.interval == default_collect_interval, file /cygdrive/c/buildbot/worker/package_win32/build/src/gc.c, line 3582
      From worker 2:	
      From worker 2:	[50132] signal (22): SIGABRT
      From worker 2:	in expression starting at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\ccall.jl:1059
      From worker 2:	Allocations: 638253607 (Pool: 638037914; Big: 215693); GC: 2048
Worker 2 terminated.
ccall                                            (2) |         failed at 2022-09-02T20:53:24.341

@gbaraldi
Copy link
Member

gbaraldi commented Sep 7, 2022

There is an rr trace for that in the rr testset.

@JeffBezanson
Copy link
Sponsor Member Author

This assertion seems odd:

Yeah I don't see why we would need that. Adding threads after startup makes that assertion straightforwardly untrue.

@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Sep 7, 2022
@JeffBezanson
Copy link
Sponsor Member Author

I don't get the assertion error running the ccall test, but instead I get

  LoadError: TypeError: in ccall argument 1, expected Int32, got a value of type Int32
  Stacktrace:
    [1] top-level scope
      @ ~/src/julia/test/ccall.jl:1098

and have also seen memory corruption during bootstrap, so something must be going wrong here.

@JeffBezanson
Copy link
Sponsor Member Author

OK the Int32 thing is just due to the IR verifier not handling cglobal, but with GC_VERIFY I get

    JULIA usr/lib/julia/corecompiler.ji

[551] signal (11): Segmentation fault
in expression starting at compiler/bootstrap.jl:10
gc_verify_tags_page at /home/jeffbezanson/src/julia/src/gc-debug.c:352
gc_verify_tags_pagetable0 at /home/jeffbezanson/src/julia/src/gc-debug.c:366
gc_verify_tags_pagetable1 at /home/jeffbezanson/src/julia/src/gc-debug.c:380
gc_verify_tags_pagetable at /home/jeffbezanson/src/julia/src/gc-debug.c:394
gc_verify_tags at /home/jeffbezanson/src/julia/src/gc-debug.c:433
_jl_gc_collect at /home/jeffbezanson/src/julia/src/gc.c:3344
ijl_gc_collect at /home/jeffbezanson/src/julia/src/gc.c:3499

@JeffBezanson
Copy link
Sponsor Member Author

JeffBezanson commented Sep 9, 2022

Ok, never mind, that has been happening for a long time at least since 1.7...
Looks like MEMFENCE is broken.

@JeffBezanson JeffBezanson added this to the 1.9 milestone Sep 13, 2022
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Sep 21, 2022
@oscardssmith
Copy link
Member

Ready to merge?

@IanButterworth IanButterworth added the don't squash Don't squash merge label Oct 5, 2022
src/init.c Outdated Show resolved Hide resolved
With dynamic thread counts, we cannot ensure this count is constant
after initialization, and we might like to even profile adding and
removing threads.
@JeffBezanson JeffBezanson force-pushed the jb/jn/mthreads branch 3 times, most recently from f48fd6c to 532ffd6 Compare October 7, 2022 18:30
@JeffBezanson
Copy link
Sponsor Member Author

@vtjnash Please re-review jl_atexit_hook --- I'm not sure what the scope of the gc unsafe region should be. It was a bit oddly nested before.

src/init.c Show resolved Hide resolved
Hook a couple functions (notably cfunction) to handle adopting
foreign threads automatically when used.

n.b. If returning an object pointer, we do not gc_unsafe_leave
afterwards as that would render the pointer invalid. However, this means
that it can be a long time before the next safepoint (if ever). We
should look into ways of improving this bad situation, such as pinning
only that specific object temporarily.

n.b. There are some remaining issues to clean up. For example, we may
trap pages in the ptls after GC to keep them "warm", and trap other
pages in the unwind buffer, etc.
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 12, 2022
@JeffBezanson JeffBezanson merged commit c63c1e4 into master Oct 14, 2022
@JeffBezanson JeffBezanson deleted the jb/jn/mthreads branch October 14, 2022 20:15
@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label Oct 14, 2022
@DilumAluthge
Copy link
Member

Is there any chance that this PR broke i686-linux-gnu on master?

@ancapdev
Copy link
Contributor

From NEWS

Threads started outside the Julia runtime (e.g. from C or Java) can now become able to call into Julia code by calling jl_adopt_thread. This is done automatically when entering Julia code via cfunction or a @ccallable entry point

Presumably this means there's now extra overhead to entering a cfunction to perform that check, and code built on that or wrappers like FunctionWrappers.jl will suffer a performance regression in 1.9?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 30, 2022

The overhead to support this was added pre-1.0, the feature is just finally enabled now

@ancapdev
Copy link
Contributor

Interesting, didn't know there was already a dead check and branch in every cfunction, thanks for clarifying.

Separate topic I guess, but that and other reasons would make it compelling to add native typed functions, and a way to bind type erased closures to such function objects with no other overhead than an indirect call and with full support of all types (i.e., not limited to what passes through the Julia C ABI).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't squash Don't squash merge multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling a Julia function from a non-Julia thread
8 participants