Note: This post is going to make me even less popular with the
Rosegarden developer community than I already am.
My topic for today ...
There are places in the codebase where ViewElements are constructed only
to be destructed shortly thereafter, sometimes immediately. Is this a
problem? Maybe, maybe not.
My particular interest is the Matrix Editor. I submitted Bug #1626 and
Merge Request #57 because I found that MatrixElements are very expensive
to construct and destruct. Not so much the objects themselves, but
adding and removing their QAbstractGraphicsShapeItems from the
QGraphicsScene. This is both documented and experimentally observable.
The solution in Merge Request #57 was to not do the adding/removing in
MatrixElement::reconfigure(), which is where the element's
positions/shapes/sizes/etc are set. Instead, two or more
QAbstractGraphicsShapeItems are permanently maintained by the
MatrixElement object, and reconfigured/hidden/shown as necessary.
I thought that would be sufficient because I believed MatrixElements
were only constructed when the user created a note, and persisted until
they were erased by her/him. So the constructor still creates and adds
the Qt objects, and the destructor removes them.
In the process of working on Feature Request #504 I discovered there is
far more "thrash" constructing/destructing MatrixElements than I
thought. For example, Matrix Editor tools such as MatrixPainter
construct a MatrixElement, but also a MatrixInsertionCommand. The latter
triggers a ViewSegment::eventAdded() call (I'm skipping several steps
here) which eventually constructs a new MatrixElement identical to the
first, and the original one is then destructed.
The above could probably be "fixed" (if fixing is necessary), but a
worse situation happens in ViewSegment::findEvent(). That constructs a
polymorphic ViewElement for the sole purpose of using it to find
matching existing ViewElements bound to the same Event, and then
immediately destructs it. (In an excellent example of good coding
practice, the local variable is appropriately named "dummy".) The issue
is that findEvent() is called several times per note during many Matrix
Editor user interactions, and with lots of notes and expensive
constructors/destructors ... well, the problem is obvious.
Again, it could be argued that none of this matters. Hardware is very
fast these days, users haven't complained about performance, and I only
did when it impacted me working with many thousands of notes at once. So
maybe the solution is to implement a wait cursor that shows up when
needed and move on to more important bugs (IMO having the program hang
for 15-30 seconds or more with no feedback to the user is unacceptable).
But as I've written before, I'm an optimization freak -- appropriate
optimization, not premature (that's well and correctly known as the root
of all evil) -- and all of the above bothers me. As an immediate remedy
I'm going to add a static CachedFactory (is that a real design pattern?)
to the MatrixElement class which will keep a vector of
QAbstractGraphicsShapeItems (actually two, one of QGraphicsRectItems
for notes and one of QGraphicsPolygonItems for percussion). When the
MatrixElement constructor (actually the reconfigure() method) needs one
it will pop it off and return it, or if the vector is empty do the
expensive construction and scene::addItem(). Likewise the MatrixElement
destructor will return the items, and the factory will call hide() and
pop them back onto the vector for reuse.
This will solve a lot of the inefficiency, and maybe that's enough. But
I'm also still thinking about ViewSegment::findEvent(). What if Events
had an std::set of ViewElements that referenced them? Each ViewElement
constructor would register itself and be added to the set, and its
destructor would unregister and it would be removed. Then findEvent()
could just return the set's begin() iterator instead doing the current
equal_range() lookup. The important point is not the lookup, but that
the expensive "dummy" ViewElement object wouldn't be needed.
I get the feeling that in general the RG architecture frowns on keeping
duplicate copies of data, which is a design philosophy I support.
(There's always the possibility that the copies can get out of sync,
with disastrous results.) But I also temper my support with pragmatism.
RG makes very heavy use of STL design patterns (another thing I like).
But it's cases like this where a full understanding of the implications
and trade-offs need to be considered.
Note that I never planned on doing this kind of deep surgery on the RG
code. I came here to fix one bug and implement one feature, yet in doing
so I keep falling deeper and deeper down the rabbit hole. I've tried to
stay strictly within the Matrix Editor sources (with the exception of
removing the unneeded menus and buttons in other windows that brought up
the Percussion Editor variant of it I deprecated) but the ideas in this
post go way further. Please understand that I haven't gone looking for
(IMO) problems like the above -- they just seem to find me.
Comments/reactions/criticisms/suggestions welcome. And I obviously
understand that there's a good chance anything I do (including the merge
requests I've already submitted) will end up living forever only in my
own repository fork. I guess that's OK -- I really like Rosegarden, and
if and when I hammer into the shape I need I plan on using it heavily.
My college advisor used to tell me, "The dark side of ability is
choice". If I didn't have the chops (I claim) to make these changes I'd
have to accept the program as-is, or move to another software package.
Probably would have been a lot easier in the long run.
_______________________________________________
Rosegarden-devel mailing list
Rosegarden-devel@lists.sourceforge.net - use the link below to unsubscribe
https://lists.sourceforge.net/lists/listinfo/rosegarden-devel
