#banshee channel, 10th of August, 2014

gistfile1.txt

22:57 <bugbot> New banshee bug 734590 filed by xDarkice.
22:57 <bugbot> xDarkice added attachment 283041 to bug 734590
22:57 <bugbot> Attachment https://bugzilla.gnome.org/attachment.cgi?id=283041&action=edit patch, Proposed patch, 0001-gstreamer-Don-t-dispose-GValue.patch
22:57 <bugbot> Bug https://bugzilla.gnome.org/show_bug.cgi?id=734590 normal, Normal, ---, banshee-maint, UNCONFIRMED, Don't dispose managed GValue instance
-!- Day changed to Monday, August 11, 2014
00:06 <knocte> xDarkice: ping
00:17 <xDarkice> knocte: pong
00:19 <knocte> xDarkice: just saw your patch
00:19 <knocte> and I'm slightly concerned about it; an IDisposable in .NET is an object that should be disposed, always, as in, not doing it is a programmer error
00:20 <xDarkice> Value has no owned mechanism, but if it had then it would be set to false here
00:21 <knocte> yeah, so, either we need an owned/unowned mechanism in GValue, and the GValue that bindinator returns for that is unowned, or
00:21 <knocte> or we make an UnownedGValue which is not IDisposable
00:22 <knocte> don't you think?
00:22 <xDarkice> you shouldn't actually dispose most GValues
00:22 <knocte> well, then we should make GValue not be IDisposable, and create a new OwnedGValue
00:22 <knocte> and adjust APIs properly
00:25 <xDarkice> the thing is GValue is not refcounted, the current dispose functionality should have been called Unset and make Value not implement IDisposable
00:28 <knocte> ok, then I think you should just make a PR with that in glib-sharp
00:30 <knocte> xDarkice: and what if you forget to call Unset(), would that involve a memory leak in some case?
00:30 <xDarkice> yes
00:30 <knocte> then it's correct that it implements IDisposable
00:30 <xDarkice> if you own the Value then you have to call g_value_unset
00:30 <knocte> at least for the cases in which it would be a memory leak
00:31 <xDarkice> but for the cases where you don't own it you must not call g_value_unset
00:31 <xDarkice> true
00:31 <knocte> then we definitely need an OwnedGValue and a UnownedGValue
00:31 <knocte> the former would implement IDisposable, and call Unset() inside Dispose()
00:31 <xDarkice> in gst# when the refcount of the taglist reaches 0 then the value will get freed,
00:32 <xDarkice> I think it's better to have an owned flag inside of Value and only dispose if the managed side owns the value
00:32 <knocte> but if we do that, we would have Dispose() methods that in some cases do nothing
00:33 <xDarkice> yes, but that's widely used in gtk#
00:33 <knocte> and developers would be chased by their static-analysis tools to call Dispose() in cases where it's not needed
00:33 <xDarkice> Opaque does that
00:33 <knocte> really?
00:33 <xDarkice> yes
00:33  * knocte reads Opaque.cs
00:36 <xDarkice> Dispose calls Unref which the generator generates as Unref () { if (Owned) { do_unref (Handle); Owned = false;}
00:38 <knocte> meh
00:40 <knocte> xDarkice: that doesn't happen in https://github.com/mono/gtk-sharp/blob/master/glib/Opaque.cs , so what are you describing?
00:40 <xDarkice> Dispose is virtual and subclasses can overwrite the Dispose function to implement ref/unref functions
00:41 <xDarkice> that's already covered in the generator
00:42 <xDarkice> https://github.com/mono/gtk-sharp/blob/master/generator/OpaqueGen.cs#L111
00:46 <knocte> xDarkice: that line is around spitting Free()/Unref() methods, not Dispose()
00:46 <knocte> I only see Dispose() generation if it's deprecated
00:46 <xDarkice> yeah but dispose will call that
00:46 <knocte> can I see a particular example of a Dispose() method that calls Unref() or Free()
00:46 <knocte> ?
00:48 <knocte> because I don't see in that code a Dispose() method generated with a call like that
00:48 <xDarkice> Well, Dispose in Opaque.cs will set Raw to IntPtr.Zero. Then the property in Raw will call Unref. If the object is not owned then Unref will do nothing
00:49 <xDarkice> In that case calling Dispose just set Raw to IntPtr.Zero
00:49 <xDarkice> so it's not really doing anything at all
00:50 <xDarkice> assuming the subclass is overriding Unref
00:51 <knocte> ok I see
00:52 <knocte> then we should implement something similar for GValue
00:52 <xDarkice> so at least for Opaque calling Dispose must not lead to a call to Unref ;)
00:52 <xDarkice> yes
00:52 <knocte> same mechanism but calling Unset instead of Unref
00:52 <xDarkice> right
01:00 <xDarkice> There should also be a destructor in Value for automatic resource management
01:04  * knocte nods
01:15 <xDarkice> actually, I don't think it makes sense to call dispose everywhere when you have automatic resource management, I would only call it when the resource takes a lot of memory
01:29 <knocte> so you think GValue should not be IDisposable, and that its finalizer should Unset it if its owned?
01:29 <RAOF> Providing you also tell the GC that said object creates memory pressure.
01:30 <xDarkice> no, you should have both
01:30 <knocte> right, ok, so like in CairoSharp, no?
01:30 <xDarkice> yes kinda
01:31 <knocte> thing is, in CairoSharp waiting for the finalizer to kick in to dispose is actually considered a bug, as we have some traces that alert us about it
01:31 <xDarkice> why is it considered a bug?
01:32 <xDarkice> it's faster to implicitly call Dispose but if you don't care then the destructor should be fine
01:32 <xDarkice> and dispose is non-terministic of course
01:37 <RAOF> That's idiomatic C#
01:38 <xDarkice> ;)
01:38 <RAOF> I mean: it being a bug to fall back to the finaliser is idiomatic C#
01:38 <xDarkice> actually, Value is a struct and cannot have a destructor :/
01:39 <knocte> xDarkice: why is it considered a "bug"? ask the developer who wrote this line: https://github.com/mono/gtk-sharp/blob/master/cairo/CairoDebug.cs#L65 ;)
01:42 <RAOF> Really what we want is some slightly better CLR support for wrapping reference-counted unmanaged objects :)
01:42 <xDarkice> there is SafeHandle, not sure why gtk# didn't use that for implementing disposing resources
01:44 <xDarkice> and also CriticalHandle for resources that allocate much resources
01:46 <RAOF> Neither of those is really what I was thinking of.
01:46 <RAOF> Because they all involve having a finaliser.
01:47 <xDarkice> no, when implementing SafeHandle you won't need to implement your own finalizer
01:47 <RAOF> Right, but SafeHandle *has* a finaliser.
01:47 <xDarkice> true
01:47 <RAOF> As in, when it's GCd it gets copied onto the finaliser stack and then the finaliser gets run at some point, probably\.
01:49 <RAOF> (Although it does try harder to run the CriticalFinalizer, it seems)
01:49 <knocte> RAOF: so why do you not like that? is it because you think that .NET who interoperates with reference-counted unmanaged code should unref ASAP rather than when GC kicks in?
01:49 <knocte> s/.NET who/.NET code which/
01:49 <RAOF> knocte: No, it should unref *when* GC kicks in.
01:50 <RAOF> knocte: As in - you should be able to say “hey, run this (very limited) unmanaged code when you GC this object”
01:50 <knocte> RAOF: so you don't like IDisposable semantics for Unref()?
01:50 <RAOF> Not really, no. They're probably the best we can manage with the CLR as it is, though.
01:51 <knocte> yeah, as a last resource?
01:51 <knocte> ok
01:51 <RAOF> Instead of “Hey, run this (potentially unbounded) managed code that could call out to whatever the hell it wants sometime after you GC this object”
01:52 <xDarkice> that would be nice to have
01:53 <RAOF> Because reference-counting is really a limited form of GC. *Most* of the objects that are reference-counted should just die in GC rather than having to think about them.
01:53 <RAOF> Things like images, which generate a lot of memory pressure, can be IDisposable, but most of the glib stuff that's refcounted is just refcounted because that's the only form of GC glib allows.
01:54 <knocte> RAOF: so if you could add features to the CLR about this, what would you design?
01:55 <RAOF> Probably something like a type attribute [UnmanagedRefRelease("g_object_unref", handle)]
01:56  * knocte nods
01:56 <knocte> and the CLR would call it ASAP? even with more rush than GC?
01:57 <RAOF> Where the GC uses that to call "g_object_unref" with the "handle" field when it's collecting the object.
01:57 <RAOF> Oh, no. It doesn't need to be triggered faster than GC.
01:58 <RAOF> It means the GC can collect the managed object immediately, and then call g_object_unref(<the_saved_handle_value>) at its leisure.
01:58 <RAOF> Rather than having the objects persist for another GC cycle, cluttering things up.
01:59 <knocte> but with that you wouldn't solve the problem about things like images, which generate a lot of memory pressure, which should be freed with more rush than GC
02:00 <xDarkice> those should be freed manually
02:00 <RAOF> Right.
02:01 <knocte> so we would still use IDisposable? but you cannot mark some as IDisposable and some other not, so we would be in the same semantics problem
02:01 <RAOF> If you *know* you're using lots of memory (or have some other limited resource), IDisposable is the right pattern.
02:01 <knocte> but APIs don't expose that info in any way :)
02:01 <RAOF> I guess to use this automatically we'd need manual annotations for all the stuff.
02:01 <RAOF> Right.
02:02 <knocte> so we probably need to extend GI with some attrib "memory-pressure: high" lol
02:03 <RAOF> Something like that, yeah. :)
02:03 <RAOF> So, with the CLR as we have it now, I'd say the right balance is: IDisposable everywhere, falling back to finalisers is expected in most cases, and we can manually add warnings for some finalisers if desired.
02:05 <knocte> xDarkice: and how about structs that can't have finalizers? ;)
02:05 <knocte> oops, I meant "RAOF:"
02:05 <RAOF> How can we get refcounted structs?
02:05 <xDarkice> wrap them into classes ;)
02:06 <knocte> looool
02:06 <RAOF> But if we've got a struct then the GC will handle it already?
02:06 <RAOF> Because we'll just free the memory?
02:07 <RAOF> Oh, except that it won't call g_*_unref on any of the child objects?
02:07 <xDarkice> for most structs, but GValue has it's own semantics to call g_value_unset to free all resources associated with the GValue
02:07 <RAOF> So GValue *isn't* a struct.
02:07 <xDarkice> it is
02:07 <RAOF> But it can't be.
02:07 <knocte> so maybe it shouldn't, if it's owned
02:08 <RAOF> Because you've just said that we don't know the memory layout, so it can't be a struct?
02:08 <xDarkice> tricky thing is that GValue is not even refcounted
02:08 <knocte> so let's welcome OwnedGValue class then?
02:09  * knocte wonders if fucking GValue should be deprecated with some saner thing
02:09 <RAOF> Yay ugly :)
02:09 <RAOF> How do we know when we've got an OwnedGValue or a regular GValue?
02:09 <xDarkice> annotations
02:09 <knocte> transfer: full|none
02:09 <RAOF> Ah, cool. So at least we can encode that into the signature.
02:09 <knocte> aye
02:10 <RAOF> Well, that seems least terrible.
02:10 <RAOF> knocte: It sounds like GValue is super-awkward for bindings; maybe it _should_ be replaced with something more useful.
02:10 <xDarkice> yes it is
02:12 <xDarkice> I'm not even sure if we can do the owned/unowned thing, because what if there are more instances to the same GValue?
02:12 <RAOF> It's passing around GValue* and asking you to kindly not free it?
02:14 <xDarkice> it's passing around &GValue most of the time
02:16 <xDarkice> it's not problematic to free the GValue itself, but the resources associated with it
02:26 <knocte> xDarkice: how about throwing ObjectDisposedException if those other instances try to use it after
02:26 <xDarkice> but you don't know that
02:30 <xDarkice> every GValue will be a copy of the struct, but you won't know if the memory it references is still intact
02:35 <knocte> xDarkice: do something similar to this then? https://github.com/mono/gtk-sharp/blob/master/glib/ToggleRef.cs#L127
02:36 <knocte> (PendingUnrefs) and if the GValue is not in the array, it means it has been already unrefed
02:36 <xDarkice> I think the ToggleRef needs support from the native side
02:36 <xDarkice> that's something only GObjects have
02:37 <knocte> I mean the technique about having a static private list 
02:37 <xDarkice> But the whole GObject unreffing/destroying is some kind of magic for me :D
02:37 <knocte> xDarkice: then you should learn it first :)
02:39 <xDarkice> I think the difference to Opaque is that GObjects will only take 1 ref and then the handle is put into a hashtable and other objects reffering to that handle will take a WeakRef
02:40 <xDarkice> and a ToggleRef is just a callback when the native object would get unreffed
02:40 <xDarkice> but I'm not sure about this
02:44 <xDarkice> The ToggleRef thing might be for the cases where the native side took over the ref from the managed side, then the managed side knows that the object has been disposed and can handle it accordingly
02:48 <RAOF> I do believe that's what the ToggleRef is for, yes.