[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.