[Kstars-devel] AuxData and QStrings in unnamed star objects

[Khudyakov Alexey]

On Thursday 04 of June 2009 23:05:27 James Bowlin wrote:
> IIRC, the current (incorrect) implementation of AuxInfo in unnamed stars
> was a temporary placeholder.  When a star object gets destroyed we do
> NOT want to also destroy the AuxData (which the user entered).  The
> reason for this is that we are swapping stars in and out of memory, and
> this should be totally transparent to the user.
>
> The plan was to add a layer of indirection for the AuxData field
> so the actual pointers to AuxData objects would reside as values
> in a QHash that would live at least as long as the KStars program lived.
> Eventually it might even get saved to disk (does it already?) to live
> between invocations of KStars.  The keys in the hash would be unique
> identifiers in each star object so when a star is deleted and created
> it still would get the same original AuxInfo object.
>
> IMO the correct thing to do is to now implement the hash instead of
> trying to destroy AuxData correctly.  IIRC hashing AuxInfo objects was
> the one of the next things on the TODO list when we ran out of time.
>
James, much thanks for you comments. 

Interesting question is how construct unique hash for sky objects. I could not 
imagine one. And in this case there is no need to store AuxData in SkyObject 
it always could be retrieved from hash.


> Also, one way to deal with the QStrings in unnamed star objects is to
> make sure we use pointers to QStrings instead of QString objects or
> references.  Then we don't have to worry about freeing QStrings inside
> of unnamed stars.  I think (but I'm not at all sure) that this might
> also work for references.  I *think* that references are internally
> (to C++) just like pointers.  So if all of the unnamed stars are
> referencing the same QStrings, then again we don't have to worry about
> freeing QStrings when deleting unnamed stars.  In fact we might have
> relied on a trick like this.  It is possible that the QStrings are okay
> (although ugly) and the only thing that needs to be fixed is AuxInfo.
> Although if this is the case, we certainly need to comment the code to
> explain what is going on.
>
References are actually pointers in disguise.

Another approach is to store all proper names (like Dumbell nebula) in some 
kind of global hash same way as AuxData. It's possible because number of such 
names is limited. This way names need not to be stored in SkyObject.
And catalog names like M33, NGC224, HD44444 could be generated on the fly.


> When the star swapping code was being written, we did discuss using
> placement new instead of memcpy.  ISTM that it did pretty much the
> same thing as memcpy with the same benefits and disadvantages.  The
> memcpy code was already in place and we were pressed for time so IIRC,
> we did not circle back and try using placement new instead of memcpy.
> As long as it does not slow things down, I think placement new is better
> than memcpy because it is more C++-like.
>
Main problem with memcpy based approach is that constructor/destructor 
machinery is circumvented and any bookkeeping based on that wouldn't work.

It seems that problem is with suboptimal constructors. Below timing for 
DeepStarComponent::loadStaticStars

~220ms - memcpy approach
~440ms - naive approach. new/delete is used.
~290ms - optimized StarObject constructor. 

There are still room for constructors optimization. Elimination of temporary 
objects for example. However these results are not very reliable because 
patches breaks things down somewhat.


> We went to great lengths to make the star swapping code as fast as
> possible.  I'm very glad someone has taken an interest in cleaning
> it up.  There is no question that we must fix bugs, especially bugs
> that could cause crashes.  But I think it is important to try to keep
> that code as fast as possible (without crashes) even at the expense
> of appearing un-C++-like.  IMO as fast as possible then as C++ as
> possible is the ideal.

I'd say not only that code. KStars gives impression of being generally slow. 
So a lot of profiling and subsequent optimizations is required.