Re: [Orekit Developers] Multi-threading problems in OREKIT

[MAISONOBE Luc <luc.maisonobe@c-s.fr>]

Hi Thomas,

Thomas Neidhart <thomas.neidhart@gmail.com> a écrit :

> On 02/20/2012 10:22 PM, MAISONOBE Luc wrote:
>
> > Some work has been done on this topic and committed to the forge in a
> > dedicated branch. See TimeStampedGenerator and TimeStampedCache. For
> > now, no classes use these features.
> >
> > We would be happy to get some feedback about this.
>
> Dear Luc,
>
> I have looked at the branch, very nice code as always ;-), but I have
> some (minor) comments:

Thanks for reviewing.

> TimeStampedCache#ctor:
>
> safeguard for maxSlots < 1

You're right, it should be checked.

> TimeStampedCache#selectSlot:
>
> When a newly added slot is exceeding the maximum number of slots, the
> slots array is (unnecessarily) resized. Increase the initial capacity by
> 1, or add the new slot not before the evicted one is removed.

Yes. I hesitated to do that, as it could increase complexity. If this  
occurs, it will occur once and then the capacity of the slot would be  
increased to maxSlot + 1, and therefore will never be exceeded again.

What would you think about allocating the slots list with a capacity  
set to maxSlots + 1 right from the constructor (it is currently  
allocated with maxSlots). The alternative to avoid the additional  
entry implies handling the entries shift by ourselves between the new  
entry and the evicted one, taking care of which one is in the front  
since the shift is either one index forward or backward.

> Slot#entryIndex:
>
>  This if is not safe, it can lead to an IndexOutOfBoundsException
>  (actually I got one ;-):
>
>  if (cache.get(guess + 1).getQuantum() > dateQuantum) {
>
>  if the cache size is <= guess + 1, the entry is after the last entry

Good catch, thanks!

> Slot#getNeighbor:
>
> The way new entries are generated seems to have a bug. I have created
> the following test. Create a cache with a generation step of 1hour, than
> call twice getNeighbors with 1h apart dates:
>
> TimeStampedCache<AbsoluteDate> cache = createCache(10, 3600.0, 13);
>
> AbsoluteDate start = AbsoluteDate.GALILEO_EPOCH.shiftedBy(0);
> cache.getNeighbors(start);
> start = start.shiftedBy(3600);
> cache.getNeighbors(start);
>
> My expectations was to have 20 entries in the cache, but in fact there
> were 26, and the center-date for the second call was at 12:30 instead of
> 01:00. Also it would be good to not create entries if it is not
> necessary (they are already in the cache) as the generation may be
> costly for some generators?

I understand you point. I considered the case where points are read  
from a file (say a JPL ephemeris). In this case, it seemed better to  
create bunch of points instead of one point only. However, this could  
be handled at generator level and not at cache level. So the cache  
could ask for just the next point, and the few generators that know it  
is more efficient to read a whole set at once will do it. In fact, the  
API already allows it since generators can provide more points than  
requested, I simply did not fully used it.

> @Frames: To be compatible with this concept, a Frame object would also
> implement the TimeStamped interface. There would be a cache for each
> frame type, and when one wants to get the transformation between two
> frames at a given time, the corresponding Frame objects would be
> retrieved from the caches, and the transformation calculated. Do I
> understand this correctly?

No, the Frame instances would still be built as they are now so they  
can be linked together with a simple tree. The frames would contain a  
cache for the transforms, which would implement TimeStamped.

I'm not sure yet if we should set up interpolation between transforms  
at frame level and removing the specialized cache for lower level  
computations like precession-nutation model, or if we should preserve  
both levels (just like processors have level 1 and level 2 caches, and  
sometimes even level 3).

What do you think about the way multithreading is handled with  
ReentrantReadWriteLock at slots end entries level ? Does it seem  
appropriate to you ? Did you find any design or implementation problem  
on this specific topic ?

Thanks again for your review.

Luc

> Thomas



----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.