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

Create a Base.root_task method and document it #35726

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented May 4, 2020

Currently, the constant Base.roottask is not documented as part of the public API per #35715. The closest API method is Base.current_task().

Rather than documenting Base.roottask I suggest creating Base.root_task() whose implementation could change further down the road if needed. The name is consistent with the existing Base.current_task().

Currently the implementation just returns Base.roottask. Alternatively, this could call jl_get_root_task, but more work might be needed to expose it.

Root Task detection is currently used by JavaCall.jl to detect if initialization is occurring on the root Task since otherwise a segmentation fault would occur if JULIA_COPY_STACKS were not set to 1 or "yes".

Root Task detection may be needed to implement a work around for using JavaCall from @async per @vtjnash and @c42f : #35048 (comment) #35048 (comment)

@c42f
Copy link
Member

c42f commented May 5, 2020

I'm not sure we actually want to have this as a public API. If the root task is a special resource, then it'll need to be shared by all packages, so I expect we'd want to reserve it for the system to run a specialized event loop. Then the API might be something like result = run_on_root_task(f) for user defined functions f?

@mkitti
Copy link
Contributor Author

mkitti commented May 6, 2020

I agree that the root Task is a special resource, and that such a facility such as run_on_root_task should exist. Does that facility need to exist in Core, Base, or stdlib and should it be active by default? For the moment, I think we should implement it as a package.

Exposing a reference to the root task in the public API does not compromise its status as as special resource or prevent it from being shared by all packages. However, having a reference is necessary so that

  1. A package on the receiving end of run_on_root_task that implements the loop to service it would need that reference to verify it is executing on the correct Task.
  2. A package could determine if it is running on the root Task or not by comparing root_task() == current_task().
  3. A call from a package not on the root_task could use yieldto(root_task())

@c42f
Copy link
Member

c42f commented May 7, 2020

Exposing a reference to the root task in the public API does not compromise its status as as special resource or prevent it from being shared by all packages

True, but it will compromise our ability to supply a different, more intrinsically sharable or useful API in the future. Also, just getting a reference to the root task really isn't very useful by itself: it doesn't imply that you have a way to run code on that task or otherwise use it as a resource for doing anything useful!

A call from a package not on the root_task could use yieldto(root_task())

But this doesn't allow you to run code that you'd like. It just schedules the root task for whatever code the root task happens to be running already.

For it to be actually useful, the system must either guarantee that the root task is the task which runs user code at startup, or is otherwise left in a waiting state, ready to run some user-defined function. In both cases, it's about having a way to run given code on the task, not having a reference to the task data structure.

So how do we move forward? I think the best way to take action right now is to prototype a shared event loop for the root task in a package, relying on the current implementation detail of which task runs which code at system initialization in order to steal control of the computation which the root task is running. Revise plays tricks like this in Revise.steal_repl_backend. There's nothing stopping you from using Base.roottask if it helps in this; it's just not guaranteed to exist in the future. But that's a good thing - it means Base is free to settle on a really solid API once we've figured out what it should be. Part of that could be changing the way that the REPL backend works internally to make your package easier to implement. For example, running both the REPL frontend and backend on new tasks and leaving the root task blocked on a Channel, ready to run user code.

In the longer term this might turn into some official API in Base for freeing up the root task as necessary.

@tkf
Copy link
Member

tkf commented May 7, 2020

Is isroottask() = roottask === current_task() all what JavaCall.jl needs at the moment? This may be an OK API to have in Base?

It may also be something useful even if we run_on_root_task have. I think run_on_root_task(f) would have FIFO behavior so that the resource would be used fairly. But, if you really don't want to let it go once you are on the root task, you can do isroottask() ? f() : run_on_root_task(f).

@mkitti
Copy link
Contributor Author

mkitti commented May 7, 2020

@tkf isroottask() would be useful to have and is a good compromise.

https://github.com/mkitti/JavaCall.jl/blame/master/src/jvm.jl#L171

Perhaps it should take an optional Task for comparison if we are interested in checking a Task other than the current one.

isroottask( task::Task = current_task() ) = Base.roottask === task

@c42f
Copy link
Member

c42f commented May 12, 2020

I'm ok with isroottask as a name. But I don't really understand why it's a generally useful function to have. I mean, I understand why you need to run on the root task for JavaCall, but the JavaCall problems are very implementation specific (to both the JVM and to the Julia runtime).

So isroottask() doesn't even express the semantics you want! If Julia were to run the logical julia-level root task on a different stack for some reason, issroottask() is not the check you would want.

Given that the test you want is very implementation-specific, I don't see what the problem is with reaching into the implementation and just doing the Base.roottask == current_task() check yourself in JavaCall?

@tkf
Copy link
Member

tkf commented May 12, 2020

It may be partially my "fault" as the first version of the "public API" PR #35715 (which may be a part of the cause behind this PR) used very strict wording. If so, I think this PR is good feedback to #35715. The reality of software is that implementation details leak sometimes (often?) and it's still useful to rely on it and build a stable interface.

@c42f
Copy link
Member

c42f commented May 13, 2020

The reality of software is that implementation details leak sometimes (often?) and it's still useful to rely on it and build a stable interface.

Yes! All I'm arguing here is that, in its current form, Base is not equipped with the right tools to provide a usable abstract API which can make JavaCall reliable. That is, anything we could provide immediately without further design work in Base is very closely tied to the current implementation.

Therefore, I'm arguing that JavaCall is the appropriate package which should be providing the stable abstraction here ("please run this java function!"), not Base ("is this task safe to run Java code on?").

Undoubtedly this means JavaCall will need to rely on implementation detail of Base for the moment, but adding isroottask will not change this fact.

@mkitti
Copy link
Contributor Author

mkitti commented May 14, 2020

The actual question is: "Base, are we running on a valid stack where the dynamic library I just loaded installed a signal handler?"

Currently, the answer is yes if:

  1. The current Task is the root Task where we never had to copy the stack OR
  2. The environmental variable JULIA_COPY_STACKS is 1 or yes.

It is not clear to me that this is necessarily specific to embedding Java.

@c42f
Copy link
Member

c42f commented May 14, 2020

"Base, are we running on a valid stack where the dynamic library I just loaded installed a signal handler?"

I was under the impression it was more complicated than this. Has the following situation changed?

#31104 (comment)

@mkitti
Copy link
Contributor Author

mkitti commented May 14, 2020

With JULIA_COPY_STACKS=1 on Linux and Mac, we can load JavaCall pretty well now even with @async

$ JULIA_COPY_STACKS=1 ~/src/julia-1.4.1/bin/julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.4.1 (2020-04-14)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using JavaCall
[ Info: Precompiling JavaCall [494afd89-becb-516b-aafa-70d2670c0337]

julia> JavaCall.init()

julia> jls = @jimport java.lang.System
JavaObject{Symbol("java.lang.System")}

julia> jcall(jls,"getProperty",JString,(JString,),"java.version")
"1.8.0_252"

julia> jls_out = jfield(jls,"out",@jimport java.io.PrintStream)
JavaObject{Symbol("java.io.PrintStream")}(Ptr{Nothing} @0x0000000002317218)

julia> @async jcall(System_out,"println",Nothing,(JString,),"Hello world");
Hello world


Confusingly, JULIA_COPY_STACKS=1 does not work on Windows (Julia crashes with it set), but JavaCall works well on that Windows x64 without it:
https://travis-ci.org/github/JuliaInterop/JavaCall.jl

It is very buggy on x86 Windows:
https://ci.appveyor.com/project/aviks/javacall-jl-6c24s/build/job/2i41xg4iqen6h3e9

@mkitti
Copy link
Contributor Author

mkitti commented May 14, 2020

I created a generic worker loop here with basic functionality: https://github.com/mkitti/TaskWorkers.jl

Much to do still. Comments welcome.

@barche
Copy link
Contributor

barche commented Oct 14, 2023

Since Qt version 6.5, this is no longer JavaCall-specific, but the same issue also affects QML.jl. There, too, as a fix we need to determine if Julia code is running on the root task (also because of the stack differences between tasks).

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