-
Notifications
You must be signed in to change notification settings - Fork 31
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
Let map
store ObserverFuncs & add precompile
#60
Conversation
This addresses a comment made in #48 (comment) by implementing storage for the ObserverFunctions added by `map`. This also adds optional precompilation support for observables, precompiling the methods based on the `T` in `Observable{T}`.
At @SimonDanisch's recommendation, I'm bumping @shashi & @piever, just to let you know about this sequence of PRs. Several have already been merged. The plan is to submit ~one PR per day and then if no concerns have arisen, merge. Hoping to then release v1.0 once the sequence (about 5 PRs after this one) is done. |
...or at least, that's my plan 😉. An alternative is to go faster. You can see the ultimate destination in the https://github.com/JuliaGizmos/Observables.jl/tree/teh/the_whole_thing branch. Starting from Simon's Dec 30th commit, this PR is the fourth in the sequence, but we've also already merged the If you look over the whole thing and like it, then we can just do them one after the other. I've got each change individually documented in the comments for each commit. |
Is this so that, if the mapper function goes out of scope, the map observable still holds on to it and does not remove it with a finalizer? What happens if in
I went over it! It looks good to me, I had questions but I think all of them were answered by the code itself. I enjoyed the fair amount of Julia internals expertise used there! Does It has fantastic docs and test updates! Also +1 for precompilation support. Thanks for making taking this to 1.0.0 to you and @SimonDanisch If you wish to open the whole thing as a PR, feel free! |
Great question! As it stands there is no change to the garbage-collection behavior, unless folks remove it manually. That's basically an indication that this PR is not taking full advantage of its own architecture; we could solve the entire resource-deallocation problem. I'll rework this a bit and see if I can solve that problem. EDIT: oh, and the thought in opening PRs one-by-one, instead of a single PR for the whole thing, is basically to provide an opportunity for discussion, even at a point after they've been merged. I don't think people typically read commit messages on their own, they seem to find the PR, so from a social perspective I thought it would be helpful to developers to have a PR for each "discussion-worthy topic." EDIT2:
Yes, that's in fact the current implementation: https://github.com/JuliaLang/julia/blob/80ace52b03d9476f3d3e6ff6da42f04a8df1cf7b/base/essentials.jl#L749-L750. Its role is to prevent the callee from being inferred with any specific type information. ( |
OK, having looked at it, from a resource allocation perspective the best description is that the current state of this PR preserves the dependencies of a julia> using Observables, Test
julia> @noinline function weakmap(val)
src = Observable(val)
dest = map(x -> 2x, src)
return WeakRef(src), WeakRef(dest)
end
weakmap (generic function with 1 method)
julia> GC.enable(false)
true
julia> wsrc, wdest = weakmap(11)
(WeakRef(Observable{Int64} with 1 listeners. Value:
11), WeakRef(Observable{Any} with 0 listeners. Value:
22))
julia> @test wsrc.value[] == 11
Test Passed
julia> @test wdest.value[] == 22
Test Passed
julia> dest = wdest.value
Observable{Any} with 0 listeners. Value:
22
julia> GC.enable(true)
false
julia> GC.gc(); GC.gc()
julia> @test wsrc.value[] == 11
Error During Test at REPL[10]:1
Test threw exception
Expression: wsrc.value[] == 11
MethodError: no method matching getindex(::Nothing)
Stacktrace:
[1] top-level scope
@ REPL[10]:1
[2] eval(m::Module, e::Any)
@ Core ./boot.jl:360
[3] eval_user_input(ast::Any, backend::REPL.REPLBackend)
@ REPL ~/src/julia-master/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:139
[4] repl_backend_loop(backend::REPL.REPLBackend)
@ REPL ~/src/julia-master/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:200
[5] start_repl_backend(backend::REPL.REPLBackend, consumer::Any)
@ REPL ~/src/julia-master/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:185
[6] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool)
@ REPL ~/src/julia-master/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:317
[7] run_repl(repl::REPL.AbstractREPL, consumer::Any)
@ REPL ~/src/julia-master/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:305
[8] (::Base.var"#872#874"{Bool, Bool, Bool})(REPL::Module)
@ Base ./client.jl:387
[9] #invokelatest#2
@ ./essentials.jl:707 [inlined]
[10] invokelatest
@ ./essentials.jl:706 [inlined]
[11] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
@ Base ./client.jl:372
[12] exec_options(opts::Base.JLOptions)
@ Base ./client.jl:302
[13] _start()
@ Base ./client.jl:485
ERROR: There was an error during testing
julia> @test wdest.value[] == 22
Test Passed
julia> dest = nothing
julia> GC.gc(); GC.gc()
julia> @test wsrc.value === wdest.value === nothing
Test Passed So in essence, even though Currently, this PR changes the failing test to a pass. However, that would be easy to change by making The intent of this PR is more about the graph of connections: from While this PR makes it possible to free julia> GC.enable(false)
true
julia> wsrc, wdest = weakmap(7)
(WeakRef(Observable{Int64} with 1 listeners. Value:
7), WeakRef(Observable{Any} with 0 listeners. Value:
14))
julia> @test wsrc.value[] == 7
Test Passed
julia> @test wdest.value[] == 14
Test Passed
julia> src = wsrc.value
Observable{Int64} with 1 listeners. Value:
7
julia> GC.enable(true)
false
julia> GC.gc(); GC.gc()
julia> @test wsrc.value[] == 7
Test Passed
julia> @test wdest.value === nothing
Test Failed at REPL[23]:1
Expression: wdest.value === nothing
Evaluated: Observable{Any} with 0 listeners. Value:
14 === nothing
ERROR: There was an error during testing
julia> src = nothing
julia> GC.gc(); GC.gc()
julia> @test wsrc.value === wdest.value === nothing
Test Passed I think fixing that is conceptually separate from this PR, but I'm happy to tackle it. |
Thanks!!
Yes I agree that this PR is a net improvement as it is. I was just asking the question I did out of curiosity, I'm fine with waiting till it is needed. :) Edit: Feel free to merge!! |
This addresses a comment made in
#48 (comment)
by implementing storage for the ObserverFunctions
added by
map
.This also adds optional precompilation support for observables,
precompiling the methods based on the
T
inObservable{T}
.Since
on
doesn't call the function right away, I don't think there's a way to precompilethem automatically (there would be, if
on
immediately called the callback, but of coursethat has issues too). So it's a step you have to perform manually.