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

remove unused roottask constant #31983

Closed
wants to merge 1 commit into from
Closed

remove unused roottask constant #31983

wants to merge 1 commit into from

Conversation

JeffBezanson
Copy link
Member

This doesn't seem to be used anywhere and must be quite obsolete.

@JeffBezanson
Copy link
Member Author

Removing an unused variable breaks everything. Classic 😂

It looks like the system image writer is depending on there being a reference to this; will fix it.

@vtjnash
Copy link
Member

vtjnash commented May 10, 2019

LOL, this would have made #31948 easier. I was wondering where that reference was popping up from. You could now just drop all references to root_task inside that file (unless you want to continue supporting packages creating references to it). I suppose you have to add a test now too to make sure it keeps working, since it's no longer implicitly tested by the sysimg build?

@mkitti
Copy link
Contributor

mkitti commented Jun 7, 2020

In JavaCall.jl, we use the roottask constant to detect if the code is running on the root task. Is there a better way?

https://github.com/JuliaInterop/JavaCall.jl/blob/81029b7220803d743bf966a059f8981973cfad3c/src/jvm.jl#L230

@vtjnash vtjnash closed this Jan 27, 2021
@StefanKarpinski
Copy link
Member

@vtjnash: is there an answer to @mkitti's question?

@mkitti
Copy link
Contributor

mkitti commented Jan 27, 2021

As long as we are not removing Base.roottask, we should be able to get by at the moment.

I suggested in #35726 that the API should be a method, Base.root_task() that parallels Base.current_task(), which would allow some flexibility in the future. We could change the underlying implementation of the method if needed. An alternative is to implement Base.isroottask( task = Base.current_task() ).

There's some concern that the concept of a root Task is an implementation detail that should not be relied upon. However, with the current implementation there is a practical need to identify the root Task for interoperability with other software such as the Java Virtual Machine. This also arose in #36064 for embedding Julia into GAP. There the solution was just to record the current Task just after initializing Julia.

Should the concept of a root Task become obsolete, one could just make Base.root_task() an alias for Base.current_task() or have Base.isroottask() = true.

@DilumAluthge DilumAluthge deleted the jb/rmroottask branch March 25, 2021 22:05
@Keno
Copy link
Member

Keno commented Apr 13, 2021

I just ran into this also. Having this magic object that needs special magic in the serializer is very unfortunate. We should get rid of it if we can.

@mkitti
Copy link
Contributor

mkitti commented Apr 13, 2021

I just ran into this also. Having this magic object that needs special magic in the serializer is very unfortunate. We should get rid of it if we can.

We just need a way to detect if the current task is the root task to prevent JavaCall.jl from crashing Julia.

The following would be sufficient if jl_get_root_task always existed.

function is_root_task(task = current_task())
    task == ccall(:jl_get_root_task, Ref{Task}, ())
end

That depends on jl_get_root_task always existing:

julia/src/task.c

Lines 753 to 757 in 107901d

JL_DLLEXPORT jl_value_t *jl_get_root_task(void)
{
jl_ptls_t ptls = jl_get_ptls_states();
return (jl_value_t*)ptls->root_task;
}

jl_get_root_task is currently only defined if JL_HAVE_ASYNCIFY.

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.

5 participants