Re: [Orekit Developers] Work In Progress for thread safety

[Thomas Neidhart <thomas.neidhart@gmail.com>]

On 04/10/2012 07:22 PM, MAISONOBE Luc wrote:
> Hi all,
> 
> Some progress has been made on thread-safety for Orekit (see issue
> <https://www.orekit.org/forge/issues/3>).
> 
> As discussed on this list and on the issue, we have set up a thread-safe
> TimeStampedCache that do not use synchronization at all but rely on more
> efficient mechanisms (read-write locks). The rationale is to have thread
> safety that works even when threads are not tightly bound to some dates
> interval. This corresponds to two major use cases:
>  - one where threads are used in a pool (see ExecutorService and the whole
>    java standard concurrent package)
>  - one where threads are created and shut down at high frequency, a new
> thread
>    being used for each request
> 
> Of course, it also supports applications where threads are completely
> under control and each thread is tightly bound to a date range.
> 
> This work started on a dedicated branch, and has now been merged back
> into master. As of writing, the general TimeStampedCache framework is in
> place for the simplest case of leap seconds handling (the UTCScale
> class). It will be extended to all other Orekit caches and can also be
> used at application level (people can for example need cached orbits).
> 
> I would be happy to get some feedback from users. If you have
> multi-threaded applications, could you check the behaviour of the
> current implementation on dates ? Be aware that for now only UTC-TAI
> relies on the new thread-safe mechanism and for example frames are not
> thread-safe yet (but will probably be soon).

Hi Luc,

I have looked into the new UTCScale version using a TimeStampedCache and
I have found some issues:

In the testMultithreading unit test, you first create 10000 random dates
with their corresponding UTC-TAI offset and compare this later inside an
ExecutorService.

My expectation was that the internal cache in the UTCScale will contain
1 slot with 40 UTCTAIOffset entries.

In fact there were 100 slots (max slot size) with 40 entries each.

After debugging into it, I found two problems:

 - in the TimeStampedCache, the quantum is stored as an int
   but it can lead to over/under-flows when doing the following
   comparison in selectSlot (and maybe somewhere else):

        if (slots.isEmpty() ||
            slots.get(index).getEarliestQuantum() > dateQuantum +
NEW_RANGE_FACTOR * neighborsSize ||
            slots.get(index).getLatestQuantum()   < dateQuantum -
NEW_RANGE_FACTOR * neighborsSize) {

   the result is, that more slots as necessary are created.
   For consistency, all uses of a quantum should be long instead of int

 - UTCScale$Generator: the generator defines its earliest and latest
   date somewhere in the range of 1960 - 2012. When providing dates
   that are outside this range, a new slot is created, which is not
   necessary, as there is no more offset data available.

   I would propose to add a method like this to the generator, and call
   it instead of cache.getNeighbors directly:

    private UTCTAIOffset[] getNeighbors(final AbsoluteDate date) {
        AbsoluteDate corrected = date;
        final AbsoluteDate latest = cache.getLatest().getDate();
        final AbsoluteDate earliest = cache.getEarliest().getDate();
        if (date.compareTo(latest) > 0) {
            corrected = latest;
        }
        if (date.compareTo(earliest) < 0) {
            corrected = earliest;
        }
        return cache.getNeighbors(corrected);
    }

   The idea is to limit the date to the available range of the cache.

 - There should be a possibility to limit the calls to
   Generator#generate. In the case of the UTCScale, there is no use in
   calling it multiple times, as it will not provide additional data,
   but there is an unnecessary overhead involved (i.e. checking the
   result).

   I have done a first test to reduce the overhead by returning an empty
   list after the first call to the generator, which works, but there
   are surely better solutions:

   private static final List<UTCTAIOffset> EMPTY =
        new ArrayList<UTCTAIOffset>();
   private boolean generated = false;

   public List<UTCTAIOffset> generate(final UTCTAIOffset existing,
                                      final AbsoluteDate date) {
       // everything has been generated at construction
       List<UTCTAIOffset> reply = generated ? EMPTY : offsets;
       generated = true;
       return reply;
   }


For the first two issues, I can provide a patch, the third one needs to
be discussed more in detail I guess.

btw. I know its early work in progress :-), but having more fine-grained
control over the cache settings would be good, i.e. having specific
default values for each cache, e.g. UTCScale cache instead of having
default values for all caches. If you agree, I can also provide patches
here.

Thomas