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

Use disable_sigint instead of sigatomic_(begin|end) #686

Merged
merged 3 commits into from May 10, 2019

Conversation

tkf
Copy link
Member

@tkf tkf commented May 2, 2019

close #684

@tkf
Copy link
Member Author

tkf commented May 2, 2019

So it seems that just switching to disable_sigint impedes Julia to inline ccalls. But we can avoid it with the optimization 6ca05f4 mentioned in JuliaLang/julia#29688.

Before 6ca05f4

julia> code_warntype(PyCall.__pycall!, (PyObject, PyPtr, PyObject, Ptr{Cvoid}); optimize=true)
Variables
  #self#::Core.Compiler.Const(PyCall.__pycall!, false)
  ret::PyObject
  pyargsptr::Ptr{PyCall.PyObject_struct}
  o::PyObject
  kw::Ptr{Nothing}
  #110::getfield(PyCall, Symbol("##110#111")){PyObject,Ptr{PyCall.PyObject_struct},PyObject,Ptr{Nothing}}

Body::PyObject
1 ─ %1 = %new(getfield(PyCall, Symbol("##110#111")){PyObject,Ptr{PyCall.PyObject_struct},PyObject,Ptr{Nothing}}, ret, pyargsptr, o, kw)::getfield(PyCall, Symbol("##110#111")){PyObject,Ptr{PyCall.PyObject_struct},PyObject,Ptr{Nothing}}
│        invoke PyCall.disable_sigint(%1::getfield(PyCall, Symbol("##110#111")){PyObject,Ptr{PyCall.PyObject_struct},PyObject,Ptr{Nothing}})
└──      return ret

After 6ca05f4

julia> code_warntype(PyCall.__pycall!, (PyObject, PyPtr, PyObject, Ptr{Cvoid}); optimize=true)
Variables
  #self#::Core.Compiler.Const(PyCall.__pycall!, false)
  ret::PyObject
  pyargsptr::Ptr{PyCall.PyObject_struct}
  o::PyObject
  kw::Ptr{Nothing}
  retptr::Ptr{PyCall.PyObject_struct}
  val::Ptr{PyCall.PyObject_struct}

Body::PyObject
1 ─       $(Expr(:foreigncall, :(:jl_sigatomic_begin), Nothing, svec(), :(:ccall), 0))
│   %2  = PyCall.getfield(o, :o)::Ptr{PyCall.PyObject_struct}
│   %3  = Base.bitcast(Ptr{PyCall.PyObject_struct}, kw)::Ptr{PyCall.PyObject_struct}
│   %4  = $(Expr(:foreigncall, :(Core.tuple(:PyObject_Call, PyCall.libpython)), Ptr{PyCall.PyObject_struct}, svec(Ptr{PyCall.PyObject_struct}, Ptr{PyCall.PyObject_struct}, Ptr{PyCall.PyObject_struct}), :(:ccall), 3, :(%2), :(pyargsptr), :(%3), :(kw), :(pyargsptr), :(o)))::Ptr{PyCall.PyObject_struct}
│   %5  = Core.bitcast(Core.UInt, %4)::UInt64
│   %6  = (%5 === 0x0000000000000000)::Bool
└──       goto #7 if not %6
2 ─ %8  = $(Expr(:foreigncall, :(Core.tuple(:PyErr_Occurred, PyCall.libpython)), Ptr{PyCall.PyObject_struct}, svec(), :(:ccall), 0))::Ptr{PyCall.PyObject_struct}
│   %9  = Core.bitcast(Core.UInt, %8)::UInt64
│   %10 = (%9 === 0x0000000000000000)::Bool
│   %11 = Base.not_int(%10)::Bool
└──       goto #4 if not %11
3 ─ %13 = invoke PyCall.PyError("\$(Expr(:escape, :(ccall(#= /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:43 =# @pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, pyargsptr, kw))))"::String)::PyCall.PyError
│         PyCall.throw(%13)
└──       $(Expr(:unreachable))
4 ┄       goto #5
5 ─       goto #6
6 ─       invoke PyCall.error("\$(Expr(:escape, :(ccall(#= /home/takafumi/.julia/dev/PyCall/src/pyfncall.jl:43 =# @pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, pyargsptr, kw))))"::String, " failed"::String)
└──       $(Expr(:unreachable))
7 ┄       invoke PyCall.pydecref_(_2::PyObject)
│         PyCall.setfield!(ret, :o, %4)
│         $(Expr(:foreigncall, :(:jl_sigatomic_end), Nothing, svec(), :(:ccall), 0))
└──       return ret

@stevengj
Copy link
Member

stevengj commented May 8, 2019

What is the relationship of this PR to #574?

@tkf
Copy link
Member Author

tkf commented May 8, 2019

The first commit c030fdd in this PR is included in #574 but #574 comes with more "destructive" changes. This is a safe subset of #574.

The second commit 6ca05f4 is a fix for the possible performance regression (ccall not inlined) due to the first commit. I thought this might be appropriate given your comment #574 (comment).

src/pyeval.jl Outdated Show resolved Hide resolved
Since `Py_CompileString` is unlikely to call Julia callback, there is
no need to put it in `disable_sigint`.  `@pycheckn` for `PyEval_EvalCode`
is in `disable_sigint` because `PyErr_NormalizeException` may call the
constructor of the exception class which may invoke Julia callbacks.

This change may not be necessary but it seems to help Julia compiler
inlining `ccall`.
@tkf
Copy link
Member Author

tkf commented May 10, 2019

AppVeyor failures are the same as #687. Merging...

I'm not using squash since each commit has distinct information.

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.

Use disable_sigint instead of sigatomic_(begin|end)
2 participants