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

Allow cleanup of timer nodes #76

Merged
merged 1 commit into from
Nov 12, 2015
Merged

Allow cleanup of timer nodes #76

merged 1 commit into from
Nov 12, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Nov 12, 2015

This demonstrates one solution to #65 (comment). If you like it, there are likely more places it needs to be applied. The key step was to allow WeakRefs to be queued on _messages.

Demo (with debug_memory=true):

julia> using Reactive
INFO: Recompiling stale cache file /home/tim/.julia/lib/v0.4/Reactive.ji for module Reactive.

julia> n = every(0.1)
Node{Float64}(1.447324753682976e9, nactions=0)

julia> Reactive.nodes
WeakKeyDict{Any,Any} with 1 entry:
  Node{Float64}(1.447324765236825e9, nactions=0) => nothing

julia> n = 0
0

julia> gc()

julia> Reactive.nodes
WeakKeyDict{Any,Any} with 0 entries

julia> 

shashi added a commit that referenced this pull request Nov 12, 2015
@shashi shashi merged commit 62bb4b7 into next Nov 12, 2015
@shashi
Copy link
Member

shashi commented Nov 12, 2015

Any idea why the previous version did not work?

@timholy
Copy link
Member Author

timholy commented Nov 12, 2015

To be honest, no I don't. I tried making sure the message queue was empty before calling gc, which you would think should do the trick (it didn't). But I didn't explicitly check that the queue was empty when I called gc, so maybe there was a problem with my test.

Oh wait! Just thought of something: what if the node variable in run is what's holding the reference? You could try setting node = nothing after send_value! and see if that fixes it.

@timholy timholy deleted the teh/gc_timers branch November 12, 2015 17:49
@@ -23,13 +23,9 @@ function every(dt)
n
end

function weakrefdo(ref, yes, no=()->nothing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using this in fpswhen below as well.

I added this function back and tried node = nothing after send_value! in core.jl. The fps node doesn't go away still.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time looking, but I confess I don't really understand the design of fps well enough to be of any use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic idea of fps is that it will fire at most rate times a second. If a signal depending on this fps signal takes longer than 1/rate seconds to update (for example drawing a complex Compose image), it will try to give you the best possible frame rate. The implementation schedules an update 1/rate seconds from the current update (during the current update) to accomplish this. The fps signal itself contains the time deltas between the previous update and the current one...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful, thanks. So the user gets enough information to make the decision, for example, whether to show the next frame of the movie or to skip ahead to the frame that should have been scheduled at the current moment? That's very nice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or compute the correct frame from a function such as update(current_frame, dt) -> next_frame. The animation would then just be: foldp(update, init_frame, fps(60)), of course you are also free to use time() and map with fps.

@timholy
Copy link
Member Author

timholy commented Nov 14, 2015

OK, this is very strange. On my machine, I can get cleanup (either on the current next branch or on the older 2acb72e, before this PR got merged) with fps(0.01) or fps(0.001). It often takes multiple gc calls, but eventually all 3 nodes get gced.

At fps(0.1), it will clean up 1 node but seems stuck on the other two. At fps(1) or higher, I can't seem to get any cleanup.

For reference, on my machine

julia> @time gc()
  0.093956 seconds (4 allocations: 160 bytes, 99.99% gc time)

@timholy timholy mentioned this pull request Nov 14, 2015
@yuyichao
Copy link
Contributor

@timholy Sorry for not really knowing the context but does the three "node"s holding reference to each other? And do they have finalizers installed?

@shashi
Copy link
Member

shashi commented Nov 14, 2015

@yuyichao yes and yes...

@yuyichao
Copy link
Contributor

So there is a known problem that having a finalizer can delay gc (reclaim of memory). Even though this doesn't mean it will delay the finalizer (for this object!), it will delay finalizers on objects it is refering to if those finalizers are installed after the finalizers of this object.

I breifly tried to fix it sometime ago but didn't really see the expected effect (might be related to JuliaLang/julia#13988). I think I'll have a look at it again. Maybe tomorrow or later today since I have sth else to work on.

@shashi
Copy link
Member

shashi commented Nov 14, 2015

without the debug_memory flag in Reactive, these finalizers should not be installed at all. So in that case do 2 objects with references to each other get gc'd together? or do they take multiple gc interventions?

(asking out of curiosity, not relevant to figuring out this issue)

@yuyichao
Copy link
Contributor

In that case they should be freed together in a single GC pause.

@shashi
Copy link
Member

shashi commented Nov 14, 2015

That's nice! thanks!

@shashi
Copy link
Member

shashi commented Nov 15, 2015

@yuyichao does WeakRef(x) add a finalizer to x?

@shashi
Copy link
Member

shashi commented Nov 15, 2015

I think I found out what was holding a reference...

It was the channel, it keeps a reference around even after it's been take!n.

julia> Reactive._messages.data
33-element Array{Any,1}:
    (Signal{Int64}(232, nactions=1),232,Reactive.print_error)
 #undef                                                      
 #undef                                                      
 #undef                                                      
                                                  

@yuyichao
Copy link
Contributor

does WeakRef(x) add a finalizer to x?

No.

@timholy
Copy link
Member Author

timholy commented Nov 15, 2015

Oh, very good detective work!

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.

3 participants