add invokelatest to circumvent world-age problems #19784

stevengj · 2016-12-30T22:14:00Z

In cfunction callbacks (#19774) and functions that call eval or similar (#19770), one occasionally runs into problems where an obsolete version of a function is called when the latest one is desired. Currently, the workaround is to call eval, which is somewhat awkward and error-prone due to the necessity of constructing an Expr. This PR adds an invokelatest(f, args...) function that calls the latest version of f(args...).

stevengj · 2016-12-30T22:16:09Z

(I modeled the jl_invoke_latest function somewhat after the jl_call API functions, which also temporarily reset the world age. One thing that confused me: I would have expected that the restoration world_age = last_age of the original value would have to go into some kind of "finally" clause so that it is still executed when exceptions occur, but jl_call does not do this. Does the exception-handling process automatically reset the world age?)

vtjnash · 2016-12-30T23:57:44Z

base/socket.jl

@@ -584,18 +584,18 @@ function uv_getaddrinfocb(req::Ptr{Void}, status::Cint, addrinfo::Ptr{Void})
    cb = unsafe_pointer_to_objref(data)::Function
    pop!(callback_dict,cb) # using pop forces an error if cb not in callback_dict
    if status != 0 || addrinfo == C_NULL
-        cb(UVError("uv_getaddrinfocb received an unexpected status code", status))
+        invokelatest(cb, UVError("uv_getaddrinfocb received an unexpected status code", status))


our libuv callbacks should never be invoking callbacks directly. we've refactored most of them, but it looks like this one still needs to be fixed.

Okay, but clearly a matter for a different PR...

yuyichao

LGTM other than the minor issues commented above.

I also think it's useful to have a version of cfunction that doesn't capture the world (and therefore might do conditional dynamic dispatch).

yuyichao · 2016-12-31T12:16:14Z

src/gf.c

+    argv[0] = f;
+    for(int i=1; i<nargs+1; i++)
+        argv[i] = jl_fieldref(argtuple, i-1);
+    size_t last_age = jl_get_ptls_states()->world_age;


Should be a jl_ptls_t ptls = jl_get_ptls_states(); at the beginning.

I think you fixed another one....

Yeah, I fixed the fix

yuyichao · 2016-12-31T12:16:57Z

src/julia.h

@@ -1381,6 +1381,7 @@ STATIC_INLINE int jl_vinfo_usedundef(uint8_t vi)

 JL_DLLEXPORT jl_value_t *jl_apply_generic(jl_value_t **args, uint32_t nargs);
 JL_DLLEXPORT jl_value_t *jl_invoke(jl_method_instance_t *meth, jl_value_t **args, uint32_t nargs);
+JL_DLLEXPORT jl_value_t *jl_invoke_latest(jl_value_t *f, jl_value_t *argtuple);


Do we really need to make this a C api? I think we can leave it out so that changing the implementation (likely) won't be a breaking change at all.

stevengj · 2016-12-31T13:37:20Z

I guess Compat for this ~~will be as simple as invokelatest(f, args...) = eval(Expr(:call, f, args...))~~, though that won't work if one of the args is a Symbol or Expr. Maybe invokelatest(f, args...) = eval(Expr(:call, QuoteNode(f), map(QuoteNode,args)...)).

stevengj · 2016-12-31T18:18:55Z

Can we get this in before the feature freeze?

vtjnash

jl_f__apply_pure would be a better model for how to implement this. But I'm not sure I like this function (that's why I didn't include it in the PR and went with the more annoying eval call), or will go with a different way entirely for making cfunction work. We'll definitely put something in before the feature freeze though.

If we do go with this, it should have a not-pure guard at the top (

julia/src/task.c

Line 409 in c38a5a3

jl_error("task switch not allowed from inside staged nor pure functions");

)

stevengj · 2017-01-02T21:08:37Z

I did look at jl_f__apply_pure in looking at how to extract the tuple elements. The main difference seems to be that jl_f__apply handles argument containers besides tuples (e.g. arrays), which didn't seem worth it here. But I'll change it to use jl_f__apply if you prefer.

stevengj · 2017-01-02T21:17:36Z

(Even if there are better ways to do cfunction, I still think it would be good to have an invokelatest function for easy fixes when world-age problems arise.)

stevengj · 2017-01-04T13:09:44Z

@vtjnash, does it look good now, aside from apparently unrelated Travis problems?

stevengj · 2017-01-05T19:03:35Z

Rebased. Good to merge?

ararslan · 2017-01-10T21:30:38Z

Bump, @vtjnash

vtjnash · 2017-01-11T02:29:11Z

I think I want to go a different direction with this (https://discourse.julialang.org/t/proposal-for-a-first-class-dispatch-wrapper/1127/2?u=jameson), which is why I've been slow here.

stevengj · 2017-02-07T17:36:50Z

Rebased. @vtjnash, are you okay with this PR now (cf. #20497)?

stevengj · 2017-02-11T20:00:33Z

Ping @vtjnash.

JeffBezanson · 2017-04-25T14:48:28Z

Bump. It seems we should merge this, since otherwise people will just define

invokelatest(f, x) = eval(current_module(), Expr(:body, Expr(:return, Expr(:call, f, QuoteNode(x)))))

which doesn't seem better.

stevengj · 2017-04-25T19:33:29Z

Rebased.

vtjnash · 2017-04-25T19:40:10Z

I'm just bike-shedding, but should we call this applylatest instead? (since it uses the API of apply, not invoke)

JeffBezanson · 2017-04-25T19:57:47Z

It takes separate arguments, not a container, so it is invoke-like.

ararslan · 2017-04-25T20:00:12Z

Can this still make it into 0.6? Kind of late now, but it's been in the works since before the planned feature freeze. (cc @tkelman)

JeffBezanson · 2017-04-25T20:08:59Z

I suddenly became very interested in this when I realized what huge overhead making and eval'ing expressions is adding to remote operations.

I'll let Jeff review

amitmurthy · 2017-04-26T03:14:04Z

Will this be applicable here - https://github.com/JuliaString/Format.jl/blob/newmaster/src/cformat.jl#L6 - too? If so, an update of this section - https://docs.julialang.org/en/latest/manual/methods.html#Redefining-Methods-1 - stating performance implications of a second eval and a fix via invokelatest would be helpful, else folks are going to end up doing a second eval anyway.

JeffBezanson · 2017-04-26T15:47:44Z

Yes looks like it would also be applicable there. I'm not sure how much we want to commit to this function, but I guess it would be ok to mention in the docs.

tkelman · 2017-04-26T16:01:02Z

Are we sure we want it exported?

stevengj · 2017-04-26T16:15:01Z

I can change it to a non-exported function and remove it from NEWS if that is what people prefer for 0.6.

StefanKarpinski · 2017-04-26T16:48:43Z

I think @JeffBezanson and @vtjnash's call, but it seems like an option.

JeffBezanson · 2017-04-26T17:07:43Z

I'd rather not export it (especially at this point in the release cycle).

vtjnash · 2017-04-26T17:56:42Z

I would keep the NEWS entry

vtjnash · 2017-04-26T18:00:45Z

src/builtins.c

+{
+    jl_ptls_t ptls = jl_get_ptls_states();
+    size_t last_age = ptls->world_age;
+    ptls->world_age = jl_world_counter;


Can you add a if (!ptls->in_pure_callback) guard on this line?

You mean like

if (ptls->in_pure_callback) jl_error("invokelatest cannot be used in a generated or pure functions");

similar to jl_toplevel_eval_in or jl_switchto?

No error is necessary in this case. It just should ignore jl_world_counter.

Okay, done, though I don't really understand why this is the case.

It's mostly just by-definition. A pure function is one for which the world-counter has no effect. In those other examples, those two functions try to change jl_world_counter, so they have to throw an error. In this example, it's only trying to read jl_world_counter, which is OK, as long as it can't see the value change.

vtjnash · 2017-04-26T18:02:32Z

base/essentials.jl

+
+Calls `f(args...)`, but guarantees that the most recent method of `f`
+will be executed.   This is useful in specialized circumstances,
+especially `cfunction` callbacks that may extract a Julia `Function`


Doesn't need to mention cfunction anymore, it's not relevant there anymore. But perhaps should add a mention of long-running event loops?

I was thinking of something like JuliaGizmos/Reactive.jl#131, where it's not calling eval, but it wants to be able to react when other Tasks call eval.

stevengj · 2017-04-26T18:33:04Z

Removed the export, updated the docstring, and added an in_pure_callback check.

stevengj requested review from vtjnash and yuyichao December 30, 2016 22:14

vtjnash reviewed Dec 30, 2016

View reviewed changes

stevengj mentioned this pull request Dec 31, 2016

world counter problem calling Julia functions from inside cfunctions (since pull #17057) #19790

Closed

yuyichao approved these changes Dec 31, 2016

View reviewed changes

stevengj mentioned this pull request Dec 31, 2016

callback functions see "old world" in 0.6 #19774

Closed

stevengj force-pushed the invokelatest branch from 1270f03 to 839336f Compare December 31, 2016 13:32

vtjnash previously requested changes Jan 1, 2017

View reviewed changes

This was referenced Jan 4, 2017

run each Expr(toplevel) argument in the newest world #19861

Merged

REPL calls display_error in obsolete world #19864

Closed

stevengj force-pushed the invokelatest branch from 2b49077 to 222e63f Compare January 5, 2017 19:03

stevengj force-pushed the invokelatest branch from 222e63f to 59a96fa Compare January 7, 2017 19:24

yuyichao mentioned this pull request Jan 21, 2017

minimal pass at getting cfunction to always run in the newest world #20167

Merged

stevengj force-pushed the invokelatest branch from 59a96fa to 53e54a8 Compare February 7, 2017 17:36

vchuravy requested a review from vtjnash February 12, 2017 05:18

JobJob mentioned this pull request Feb 21, 2017

fix tests on julia v0.6-dev JuliaGizmos/Interact.jl#158

Merged

vtjnash mentioned this pull request Apr 4, 2017

World age failure when running GtkReactive demo #21141

Closed

JobJob mentioned this pull request Apr 15, 2017

World age JuliaGizmos/Reactive.jl#131

Closed

stevengj added 2 commits April 25, 2017 15:33

slight optimization

6d35e48

change to define builtin jl_f__apply_latest

9515215

stevengj force-pushed the invokelatest branch from 53e54a8 to 9515215 Compare April 25, 2017 19:33

JeffBezanson mentioned this pull request Apr 25, 2017

simpler and faster message (de)serialize #21543

Merged

JeffBezanson added this to the 0.6.0 milestone Apr 26, 2017

vtjnash reviewed Apr 26, 2017

View reviewed changes

stevengj added 2 commits April 26, 2017 14:27

don't export invokelatest, update docstring

1c2b7e9

guard invokelatest from being called in pure functions

485b334

stevengj added 3 commits April 26, 2017 14:33

fix test

5d4a569

ignore jl_world_counter if in_pure_callback

929b7e1

update manual to mention invokelatest

fa759f4

JeffBezanson merged commit 2815324 into JuliaLang:master Apr 27, 2017

stevengj deleted the invokelatest branch April 27, 2017 13:12

stevengj mentioned this pull request Apr 27, 2017

add invokelatest from Julia 0.6 JuliaLang/Compat.jl#352

Merged

jverzani mentioned this pull request May 15, 2017

call invokelatest in lambdify symengine/SymEngine.jl#90

Merged

omus mentioned this pull request Jun 14, 2017

Switch to using invokelatest JuliaTesting/Mocking.jl#20

Merged

add invokelatest to circumvent world-age problems #19784

add invokelatest to circumvent world-age problems #19784

Conversation

stevengj commented Dec 30, 2016

stevengj commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Dec 31, 2016 • edited Loading

stevengj commented Dec 31, 2016

vtjnash left a comment

Choose a reason for hiding this comment

stevengj commented Jan 2, 2017

stevengj commented Jan 2, 2017

stevengj commented Jan 4, 2017

stevengj commented Jan 5, 2017

ararslan commented Jan 10, 2017

vtjnash commented Jan 11, 2017

stevengj commented Feb 7, 2017 • edited Loading

stevengj commented Feb 11, 2017

JeffBezanson commented Apr 25, 2017

stevengj commented Apr 25, 2017

vtjnash commented Apr 25, 2017

JeffBezanson commented Apr 25, 2017

ararslan commented Apr 25, 2017

JeffBezanson commented Apr 25, 2017

amitmurthy commented Apr 26, 2017

JeffBezanson commented Apr 26, 2017

tkelman commented Apr 26, 2017

stevengj commented Apr 26, 2017 • edited Loading

StefanKarpinski commented Apr 26, 2017 • edited Loading

JeffBezanson commented Apr 26, 2017

vtjnash commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Apr 26, 2017 • edited Loading

stevengj commented Dec 31, 2016 •

edited

Loading

stevengj commented Feb 7, 2017 •

edited

Loading

stevengj commented Apr 26, 2017 •

edited

Loading

StefanKarpinski commented Apr 26, 2017 •

edited

Loading

stevengj commented Apr 26, 2017 •

edited

Loading