Re: [Orekit Developers] Frames and WeakHashMap

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

On 10/04/2012 09:42 PM, Evan Ward wrote:
> Hi,
> 
> First, as this is my first time posting to this list, allow me to
> introduce myself. I'm an engineer working for the US Naval Research Lab
> in astrodynamics. My interests include orbit determination, global
> sensitivity/uncertainty analysis, and ice cream. :)

Hi Evan,

sorry for the late reply, but also from my side a welcome to the
community, but we already had some communication on the issue tracker.

> Thanks to the Orekit team for a well-designed and accurate library.
> 
> While looking in to concurrent orbit propagation I noticed several
> issues relating to how Frame and FramesFactory use WeakHashMaps. After
> reading up on WeakHashMap I learned it has two unintuitive attributes:
>   - WeakHashMap is *synchronized* on get and put operations. It calls
> getTable() which calls expungeStaleEntries() which is synchronized on a
> final member of the class.
>   - WeakHashMap uses WeakReferences to store the *key* and not the value.
> 
> In Frame.findCommon(...) use of WeakHashMap causes contention even when
> the common frame is already known because of the synchronized get(..).
> In my highly concurrent test case I noticed a speed up when I simply
> removed the commons cache. My guess is that the logic and comparisons in
> findCommon(...) are insignificant compared to the floating point
> operations in computing the Transform. I propose removing the commons
> map or looking in to using a ConcurrentHashMap (Java 6).
> 
> In FramesFactory a WeakHashMap is used to cache frames as they are
> created, with a Predefined as the key. Once created, Frames are never
> removed from the cache because every Predefined is static final (an
> enum). This is not a pressing issue, but it appears that the intent is a
> Frame would be garbage collected once the application is done with it.
> If the current state is acceptable I propose using a plain HashMap to
> avoid the extra overhead. If garbage collecting old Frames is important,
> perhaps a SoftReference as the value would work.

Could you open an issue for this? Maybe you even have a patch ready?

Thanks in advance,

Thomas