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

WIP: Direct binding deprecation warnings to logging system #25413

Closed
wants to merge 2 commits into from

Conversation

c42f
Copy link
Member

@c42f c42f commented Jan 5, 2018

This is a first attempt to direct the output of jl_binding_deprecation_warning to the logging system. This is a bit of a challenge as this depwarn may be generated from contexts where calling back into julia isn't allowed. I don't yet know what these contexts are and when they are active. @vtjnash suggested working around this by emitting the warning from a different task which is what I've done here.

This approach works, but I'm not really satisfied with the current implementation because

  1. It's non intuitive. I really expect and want warnings to be synchronous by default.
  2. It's fragile because the C code has to reproduce the handling of the task list which is normally only done in julia code. This is kind of ok for the simple Workqueue variable, but to make it work with @sync blocks becomes quite nasty (it would require allocating an ObjectIdDict from the C side, and inserting into it. All of which is defined in julia which we're supposed to not be calling into from this context.)
  3. Due to the above, @sync blocks don't work so you can't collect async logs using with_logger().
  4. Warnings can be lost if julia exits before the warning tasks get a chance to execute.

I've got some ideas on what to try next

  • make the call appear synchronous by running the task immediately with jl_switchto. This won't work if we're inside a finalizer or function generator, but maybe that's ok.
  • If someone could list the various circumstances under which the runtime C code can't call into julia that would be immensely helpful. I think @vtjnash mentioned that sometimes streams can't be flushed. I also ran into problems naively calling these warnings synchronously from within C functions called from inference. And it's clear that finalizers are severely constrained.

Advice appreciated - anything done here will be reused to convert the rest of the user visible logging from the C code to the logging system.

@c42f c42f added the logging The logging framework label Jan 5, 2018
@JeffBezanson
Copy link
Sponsor Member

I'd prefer not to work too hard on this --- I see deprecation warnings as kind of a marginal thing, and the less complexity spent on them the better.

@c42f
Copy link
Member Author

c42f commented Jan 5, 2018

Sure, I'd like this to be as simple as possible. But I'd also like it to exist. Any ideas for reducing the complexity would be great.

Or do you think we should just not bother doing this? Or do it later?

Keep in mind that this isn't just about deprecation warnings, the larger and more worthwhile problem is to let users and developers intercept all log events from the runtime in a structured way. With a bit more work there could also be tangible benefits for compiler development. As just one example, it would be nice to have a neat and efficient way to capture the TRACE_COMPILE output as a stream of function compilation events without recompiling julia.

* Add async support for jl_log(), for logging in contexts where calling
  into julia code is not permitted.
* Modify jl_binding_deprecation_warning() to use this.

TODO:
* Figure out a way to make adding to the work queue less fragile if
  possible.
* Tests
This is effectively synchronous logging, but the new task allows us to
have a clean context in which to call into julia.
@c42f
Copy link
Member Author

c42f commented Jan 16, 2018

I've had another crack at this by calling jl_switchto directly with the temporary task, then yeilding back directly to the task originating the log message. So far I like the look of this a lot better - it appears synchronous to the user and avoids the brittleness of duplicating any of the work queue stuff on the C side which was really quite ugly.

Also rebased to fix conflicts due to #20641.

@JeffBezanson
Copy link
Sponsor Member

I don't think we should do this at all. Binding resolution is not a good place to run large amounts of arbitrary julia code, and @vtjnash points out that doing task operations from here is probably not valid. These are just deprecation warnings; all we need to do is get some message on the screen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging The logging framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants