[Spice-devel] Refactory and spaghetti code

Tue Nov 17 06:50:08 PST 2015

On Tue, Nov 17, 2015 at 07:27:22AM -0500, Frediano Ziglio wrote:
> Cfr https://en.wikipedia.org/wiki/Spaghetti and
> https://en.wikipedia.org/wiki/Spaghetti_code!
> 
> As today part of the team involved in the refactoring review is on holiday I
> started looking at the average direction and state of this refactoring.
> 
> I looked at different angles like:
> 1- where we are;
> 2- what are actually doing;
> 3- what is the status of final code, looking at last commit in the branch.
> 
> Honestly I'm not that happy on the status.
> 
> 1- we just started the refactoring. The reason is that code is quite old,
> there are plenty of dependency here and there, the code is mainly in a
> single file hard to understand full of small/medium/large functions.
> 
> 2- actually we are trying to split this huge amount of code into separate
> files each handling a specific piece of functionality. The branch could be
> split in 3 pieces:
>  2.1- split into functionality files (actual one, about 150 patches)
>  2.2- remove global states;
>  2.3- encapsulate in gobject/glib.
> After 2.1 should be easier to understand where to edit a functionality and
> as dependencies should be less easier to add new ones or fix them.  I
> repeat... SHOULD. If this were wrong after 2.3 code should not look that
> much as spaghetti code!  Don't misunderstand me, the final code is better
> but IMHO far from what we want to probably achieve.

I don't think 2.1 aimed to reach a 'perfect' separation of red_worker.c code
in separate files, and that managing to split it was enough of a
challenge. I'm not surprised if some bits stayed behind and were not
moved if they did not get too much in the way.
And I don't think anyone would pretend the work is done after 2.3, some
things were improved, but there are still quite a few things to improve
there! Personally, there are so many things to be done for 2.3 (and 2.4,
2.5, ...) that I've mainly focused on making private data really private,
which should then help when looking at the higher level picture.

> 
> I think that when a refactoring is needed you should better understand the
> structure/features/objects which are involved in order to better define the
> relationship between them. I think this point is a bit missing.

After some discussion on IRC, my understanding is that you'd like that
we add some higher level documentation describing what the various
classes represent/what they are meant to do/not do, this makes sense to
me and would be very useful!

> Let me make
> an example of what I mean. What a RedWorker should be supposed to do? What
> is it? IMHO a RedWorker should handle a thread that manage messages between
> display and cursor channel. Easy and simple definition but it comes with
> lot of implications:
> - thread informations are RedWorker stuff;
> - should deal (receive and send) messages dispatching to display and cursor
>   channels. Once messages is dispatched the job is done! This means that
>   RedWorker should not handle any message itself!
> - should not deal with drawing object as DisplayChannel will do it. This is
>   the reason a lot of code is currently been moved from red_worker.c to
>   display-channel.c/stream.c/others. But in the final commit there are still
>   some fields which are strictly related to the display :(
> 
> 3- oh my god! Try to open red_worker.c (and many other files) and see the
> FIXME, TODO and similar. Plenty of commented out code which were removed as
> too difficult to implement (and are working in the current master!). Well..
> still to review/improve but surely not the code we want! The reason? Code
> is still in red_worker but would be better moved to other files. Note that
> separation was meant to be mostly completed after step 1 so the fact that
> after step 3 code is still there means that step 1 was not really
> completed.
> 
> One way to check for dependency in C programs are to see:
> - function calls between modules (objdump or similar tools could be
>   helpful);
> - include dependencies.
> Another thing to look are accessors functions (get/set).
> 
> Let me show some example of what I found (take a look at final commit in
> refactoring branch):
> - red_drawable_count in RedWorker. This field count drawables allocate in
>   this worker. The field is used in red_drawable_unref and red_drawable_new
>   (and some commented out code!). The RedDrawable structure is defined in
>   red_parse_qxl.h. The life of this RedDrawable object is split between
>   red_parse_qxl and red_worker creating a strong dependency between them.
>   Note that red_parse_qxl mainly deal with reading messages from VM and
>   managing structures at low level and has only an exception for this
>   RedDrawable. The only thing is able to do is incrementing the reference
>   counter while freeing and allocation is demanded to red_worker. Moving the
>   refs to an upper layer structure would solve the issue. Another comment on
>   this is why we have to keep Drawable and RedDrawable? Why don't
>   DisplayChannel allocate a Drawable from RedDrawable and use it instead of
>   dealing with two objects?
> - red_process_draw/destroy_primary_surface, mainly a bunch of calls to
>   DisplayChannel
> - red_migrate_display, why still in red_worker?
> - red_worker_get_cursor_channel/red_worker_get_display_channel. These are
>   called to finally initialize cursor/display channel allocated in
>   red_worker_new. So red_worker_new partially initialize itself, then returns
>   to red_dispatcher_init which is expected to finish the initialization of
>   these structures. Note that part of the initialization is set some callback
>   to some static functions defined in red_displatcher.c and strictly
>   associated to cursor/display channel. Now just to demonstate how not that
>   maintanable is that code try to add a function to destroy RedWorker and see
>   how many files/functions you have to edit.
> 
> I think that after a *_new function a a *_destroy *_unref could be called
> straight away. Actually this is not true. The reason why this should be
> true is that if some some reason initialization fails you should free the
> object. The reason why this is not currently true is that in some cases the
> *_new function allocate but not initialize entirely and another function is
> demanded to initliaze the object to a correct state (and at this point is
> possible to call *_destroy/*_unref).

I'd expect the _new/_destroy/_unref/_init issues to be improved when
various things are moved to gobjects. How do you suggest we should
address the other issues? Now? After merging the patches? Or something
else?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151117/3bfede20/attachment.sig>