Re: [Orekit Developers] Frames and WeakHashMap

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

Evan Ward <evan.ward@nrl.navy.mil> a écrit :

> Hi,

Hi Evan,

> 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. :)

Welcome in the Orekit community.

> Thanks to the Orekit team for a well-designed and accurate library.

Thank you very much for these kind words, they are greatly appreciated.

> 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).

I agree with you and think we should get rid of this map. It was  
introduced at the very beginning (probably 2002!) when due to speed  
issues, we tried to cache everything possible.

> 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.

No, the reason for this is much more simple: it's a plain ugly design  
error I made.

> 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.

+1 for a HashMap.

best regards,
Luc

> Thanks,
> Evan Ward



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