[Bug 796744] New: miniobject: Locking functionality complicated, unused/misused and not having any advantage over refcount-based writability

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=796744

            Bug ID: 796744
           Summary: miniobject: Locking functionality complicated,
                    unused/misused and not having any advantage over
                    refcount-based writability
    Classification: Platform
           Product: GStreamer
           Version: unspecified
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: Normal
         Component: gstreamer (core)
          Assignee: gstreamer-bugs at lists.freedesktop.org
          Reporter: slomo at coaxion.net
        QA Contact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---

While working on a solution for bug #796692, I looked at the locking
functionality in more detail and came to the conclusion that we better get rid
of it for 2.0, and ideally for 1.0 too in one way or another (see below).

To shortly reiterate what it is for and how it works:
- it decouples reference count from writability of the objects
- it requires explicit lock/unlock calls with the EXCLUSIVE flag whenever
something "takes ownership" of the object. This is basically a reference count
again, that does not decide over the object lifetime. 0/1 means object is
writable, > 1 means it is not
- it contains a second component which is the read/write mapping part. Once
locked readable, only readable locks are possible, once writeable/readwrite all
is possible. And write locks require the object to have an exclusive count of 0
or 1 to succeed

So in short: we have a second reference count for deciding writability, and at
the same time keep track of read/write mapping of the object. In an atomic way.

This was introduced for bindings: apart from C++/Rust (and possibly some other
languages) you don't have explicit control over the reference counting so could
accidentially end up with non-writable objects although they should be
writable, or you could share the object in multiple places but the refcount is
still 1.

----

So why I believe we should get rid of this is the following:

1) Nobody outside gstmemory.c, gstbuffer.c and gstminiobject.c is using these
functions but everybody who is doing anything with GstMemory should to modify
the writability of the object correct

2) Bindings can't make automatic use of this (except for C++/Rust possibly),
and nowadays don't, and also no user-code in bindings is making use of these
functions. So the original goal was clearly not achieved

3) It is only used for GstMemory right now and we can't use it for anything
else existing because that would break ABI. So even if it was useful, its
usefulness would have no effect because most code does not work with GstMemory
but other things

4) It's complicated! The implementation is (it involves an atomic spinlock
among other things), but more so the concept is. That nobody currently calls
that API although they should, should be enough proof of that. Many people
already struggle with reference counting, and this adds yet another unfamiliar
concept that is not reference counting but almost but not really

5) Current code seems to assume reference count based writability, leading to
annoying bugs (if you assume reference counting, your objects are always
writable although they shouldn't).

6) It does not bring any advantage compared to using the reference count for
writability (for the bindings case see above). If reference count == 1, you can
write and keep track of that in an instance variable. If not, you can't. Only
the check for the reference count has to be atomic, the other part not. The
only theoretical advantage of mixing both things is that you currently can't
increase the "exclusive count" if the object is write-locked. The only place
that actually checks for this to fail is in gstbuffer.c (all other code
silently fails and worse things happen), and even there it is not much of an
advantage: what it means is that someone has the memory write mapped, there
might be 1000 other places referencing the memory and one of those gave the
memory to you and now you want to write map it too. So the only thing that this
can do right now is to copy the memory, but the memory is write-mapped and some
other thread might do whatever it wants with the content while we're copying
it. There's not much gained here.

In this case the original bug is that a write-mapped memory actually has
multiple references, and that should've never happened to begin with. Anything
that is done from that point onwards is just mitigation, and not very
effective.

----

My proposal is to disable the locking functionality by default (make it noops
and use the reference count instead, which is what code currently assumes
anyway), use the struct field for keeping track for memory mapping only, and
only fall back to the current behaviour if an environment variable is used.

I didn't find any code on the Internet that is making use of the API, but quite
some code that should make use of it (our own code among other things, whenever
dealing with GstMemory) and would potentially have subtile bugs now that would
be solved by switching to reference counting instead.


Any opinions, anything I'm missing here?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.