incremental precompile correctness issues #21307

vtjnash · 2017-04-06T23:28:28Z

This should fix #21141, and includes some other correctness improvements that I've known were missing, but require rather convoluted situations to manage to trigger them.

tkelman · 2017-04-07T00:15:45Z

can any of the issues being fixed here be tested?

timholy · 2017-04-10T13:07:35Z

I tested this branch; is it supposed to work with, or without, the eval change(s) to Reactive? With unmodified Reactive, I'm still getting world-age errors when I run the GtkReactive tests.

vtjnash · 2017-04-10T13:46:23Z

I'm only addressing the assertion failure. Reactive will need to be fixed separately.

timholy · 2017-04-10T14:03:43Z

Is it going to have to stay eval? That will presumably add quite a performance hit.

I can work on the changes to Reactive, but I don't want to apply a bandaid if a deeper fix is necessary/coming.

vtjnash · 2017-04-10T14:15:09Z

This is a bug in reactive. It needs to avoid starting its event loop before finishing loading code.

tkelman · 2017-04-10T20:31:12Z

src/dump.c

+                            }
+                        }
+                        if (callee_mi->max_world == ~(size_t)0) {
+                            // if this callee is still valid, just add a backedege


backedge misspelled

tkelman · 2017-04-10T20:35:00Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

tkelman · 2017-04-10T20:51:55Z

src/dump.c

+    return jl_main_module->primary_world;
+}
+
+// repeated look up older methods until we come to one that existed


should this be "repeatedly look up" ?

tkelman · 2017-04-10T20:56:34Z

test/compile.jl

          end
          """)
    @test_throws ErrorException Core.kwfunc(Base.nothing) # make sure `nothing` didn't have a kwfunc (which would invalidate the attempted test)

    # Issue #12623
    @test __precompile__(true) === nothing

+    # Issue #21307
+    Base.require(Foo2_module)
+    @eval let Foo2_module = $(QuoteNode(Foo2_module)) # use @eval to see the results of loading the compile


supposed to be a comma here?

nanosoldier · 2017-04-10T23:31:59Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

previously we just assumed that the user didn't load any other code now this actually keeps track of which method would have been visible in the precompilation process

…ation we need to restore not only backedges directly to the new methods, but also to any methods whos backedges have not been inferred but which might propagate invalidation changes to the new code fix #21141

timholy · 2017-04-13T16:10:32Z

This is a bug in reactive. It needs to avoid starting its event loop before finishing loading code.

I'm still wrapping my head around this, but if I understand your suggestion you're not talking about Reactive's code (__init__ runs at the end of module initialization, right?), but code that's defined after Reactive is loaded. If so, I suspect this suggestion would be limiting, more so than the eval trick. Let's say that Reactive didn't launch its own event queue, and left that up to user code. Once you started working with a Reactive GUI at the REPL (necessitating that you launch the event queue), that REPL becomes "frozen" and can't have new GUIs defined. I could easily be misunderstanding, however.

Perhaps this is an unworkable/stupid idea, but what about defining a compile hint ::LATEST, aka

function connect_map(f::LATEST, output, inputs...)
    let prev_timestep = 0
        for inp in inputs
            add_action!(inp, output) do output, timestep
                if prev_timestep != timestep
                    result = f(map(value, inputs)...)
                    send_value!(output, result, timestep)
                    prev_timestep = timestep
                end
            end
        end
    end
end

LATEST is analogous to ANY. If this is no better than eval, then I can use eval.

vtjnash · 2017-04-13T16:46:25Z

Yes, I think longer-term we should do something like that. My latest attempt at defining something like this is https://discourse.julialang.org/t/proposal-for-a-first-class-dispatch-wrapper/1127/2

Let's say that Reactive didn't launch its own event queue, and left that up to user code

Yeah, that wouldn't be good either. I think it would instead do something like have add_action! restart its event loop task. I don't know exactly what that would look like, but maybe something like:

__init__() = @async backend_eventqueue()
function backend_eventqueue()
    restart[] = false
    while true
        dispatch_events()
        restart[] && (@async backend_eventqueue(); return)
    end
end

vtjnash added the bugfix This change fixes an existing bug label Apr 6, 2017

ararslan added the compiler:precompilation Precompilation of modules label Apr 6, 2017

ararslan requested a review from JeffBezanson April 6, 2017 23:52

vtjnash force-pushed the jn/incremental-correctness branch from 78ce6c1 to af61c4a Compare April 7, 2017 15:47

tkelman added the needs tests Unit tests are required for this change label Apr 10, 2017

vtjnash force-pushed the jn/incremental-correctness branch from af61c4a to 61cdfc4 Compare April 10, 2017 19:19

tkelman removed the needs tests Unit tests are required for this change label Apr 10, 2017

tkelman reviewed Apr 10, 2017

View reviewed changes

vtjnash added 4 commits April 11, 2017 15:53

fix llvm 3.3 build

7b16106

delete false pretense of support of max_world on a Method

a529199

improve correctness of incremental precompile deserialized edges

5e709e4

previously we just assumed that the user didn't load any other code now this actually keeps track of which method would have been visible in the precompilation process

add many missing backedges getting lost during incremental deserializ…

8d16ddd

…ation we need to restore not only backedges directly to the new methods, but also to any methods whos backedges have not been inferred but which might propagate invalidation changes to the new code fix #21141

vtjnash force-pushed the jn/incremental-correctness branch from 61cdfc4 to 8d16ddd Compare April 11, 2017 19:54

vtjnash merged commit bdd4d77 into master Apr 12, 2017

vtjnash deleted the jn/incremental-correctness branch April 12, 2017 13:59

timholy mentioned this pull request Apr 13, 2017

World age JuliaGizmos/Reactive.jl#131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incremental precompile correctness issues #21307

incremental precompile correctness issues #21307

vtjnash commented Apr 6, 2017

tkelman commented Apr 7, 2017

timholy commented Apr 10, 2017

vtjnash commented Apr 10, 2017

timholy commented Apr 10, 2017

vtjnash commented Apr 10, 2017

tkelman Apr 10, 2017

tkelman commented Apr 10, 2017

tkelman Apr 10, 2017

tkelman Apr 10, 2017

nanosoldier commented Apr 10, 2017

timholy commented Apr 13, 2017

vtjnash commented Apr 13, 2017

incremental precompile correctness issues #21307

incremental precompile correctness issues #21307

Conversation

vtjnash commented Apr 6, 2017

tkelman commented Apr 7, 2017

timholy commented Apr 10, 2017

vtjnash commented Apr 10, 2017

timholy commented Apr 10, 2017

vtjnash commented Apr 10, 2017

tkelman Apr 10, 2017

Choose a reason for hiding this comment

tkelman commented Apr 10, 2017

tkelman Apr 10, 2017

Choose a reason for hiding this comment

tkelman Apr 10, 2017

Choose a reason for hiding this comment

nanosoldier commented Apr 10, 2017

timholy commented Apr 13, 2017

vtjnash commented Apr 13, 2017