9 Replies Latest reply on May 13, 2008 12:57 PM by timfox

    Combined codec and packet

    timfox

      I have combined the codec and packet classes, so now the packets know how to encode and decode themselves.

      This has simplified the code a fair bit and reduced the number of lookups.

      I've also done various other tweaks/optimisations along the way (I've lost track now).

      The good news is.. we're getting there. Using MINA transport and client and server on same box, I can send over 40K 250 byte messages (non persistent). This is pretty good.

      Lots more optimisations to do though....

        • 1. Contention on MINA? WAS: Combined codec and packet
          clebert.suconic

          Yesterday... Just because I wanted to have a little better understanding of the packages.. I navigated through the code with a debugger :-) And I had seen one interesting thing.

          org.apache.mina.common.DefaultIoSessionDataStructureFactory has few synchronized collections:

           private static class DefaultIoSessionAttributeMap implements IoSessionAttributeMap {
          
           private final Map<Object, Object> attributes =
           Collections.synchronizedMap(new HashMap<Object, Object>(4));
          



          And on a test I executed today, this appeared in few threads:

          pool-3-thread-2" prio=1 tid=0x00007f611431de00 nid=0x32aa waiting for monitor entry [0x000000004444b000..0x000000004444be20]
           at java.util.Collections$SynchronizedMap.get(Collections.java:1979)
           - waiting to lock <0x00007f61296c47c8> (a java.util.Collections$SynchronizedMap)
           at org.apache.mina.common.DefaultIoSessionDataStructureFactory$DefaultIoSessionAttributeMap.getAttribute(DefaultIoSessionDataStructureFactory.java:63)
           at org.apache.mina.common.AbstractIoSession.getAttribute(AbstractIoSession.java:343)
           at org.apache.mina.common.AbstractIoSession.getAttribute(AbstractIoSession.java:339)
           at org.apache.mina.common.SimpleIoProcessorPool.getProcessor(SimpleIoProcessorPool.java:236)
           at org.apache.mina.common.SimpleIoProcessorPool.flush(SimpleIoProcessorPool.java:181)
           at org.apache.mina.common.SimpleIoProcessorPool.flush(SimpleIoProcessorPool.java:1)
           at org.apache.mina.common.DefaultIoFilterChain$HeadFilter.filterWrite(DefaultIoFilterChain.java:644)
           at org.apache.mina.common.DefaultIoFilterChain.callPreviousFilterWrite(DefaultIoFilterChain.java:467)
           at org.apache.mina.common.DefaultIoFilterChain.access$7(DefaultIoFilterChain.java:464)
           at org.apache.mina.common.DefaultIoFilterChain$EntryImpl$1.filterWrite(DefaultIoFilterChain.java:835)
          


          Shouldn't this be using concurrent?

          Are we (including Trustin) aware of that specific possible contention?


          • 2. Re: Combined codec and packet
            trustin

            Yes. The default attribute map is a synchronized hash map because concurrent hash map consumes more memory as the number of the connections grows. You can change the default map by implementing your own IoSessionDataStructureFactory and setting its new instance in IoAcceptor or IoConnector.

            Hmm, or MINA could provide a concurrent hash map backed implementation out of the box (although it will not be the default).

            Please let me know if we can decrease the memory consumption with concurrent hash map. That would be the best solution.

            • 3. Re: Combined codec and packet
              clebert.suconic

               

              Yes. The default attribute map is a synchronized hash map because concurrent hash map consumes more memory as the number of the connections grows.


              Is it really relevant? It will just provide partitioned almost empty HashMaps to avoid the concurrency on a single object.

              And.. isn't this synchronization limiting the number of connection you can reach up to as it is a contention point?

              • 4. Re: Combined codec and packet
                timfox

                I am trying to remove or at least minimise our usage of session attributes anyway so this may be moot.

                • 5. Re: Combined codec and packet
                  clebert.suconic

                   

                  "timfox" wrote:
                  I am trying to remove or at least minimise our usage of session attributes anyway so this may be moot.


                  Well.. if we need to keep them.. we can just replace the IoSessionDataStructureFactory by a Concurrent implementation.

                  • 6. Re: Combined codec and packet
                    trustin

                     

                    "clebert.suconic@jboss.com" wrote:
                    Yes. The default attribute map is a synchronized hash map because concurrent hash map consumes more memory as the number of the connections grows.


                    Is it really relevant? It will just provide partitioned almost empty HashMaps to avoid the concurrency on a single object.

                    And.. isn't this synchronization limiting the number of connection you can reach up to as it is a contention point?


                    Yes, the memory consumption matters when the number of connection becomes very large (e.g. 10000.) MINA team got complaints as soon as we switched to ConcurrentHashMap and that's why we switched back to synchronized HashMap. In most cases, the number of threads which accesses the synchronized map will be less than 3, so it is often OK to use a synchronized map.

                    • 7. Re: Combined codec and packet
                      timfox

                       

                      "trustin" wrote:

                      Hmm, or MINA could provide a concurrent hash map backed implementation out of the box (although it will not be the default).


                      I like the idea of MINA having some config switch boolean "useConcurrentSessionMap" which if true uses a ConcurrentHashMap and if false uses the synchronized map. Then the user can choose which to use based on their number of expected connections and available RAM. This can default to false to preserve current behaviour.

                      • 8. Re: Combined codec and packet
                        clebert.suconic

                         

                        "timfox" wrote:
                        "trustin" wrote:

                        Hmm, or MINA could provide a concurrent hash map backed implementation out of the box (although it will not be the default).


                        I like the idea of MINA having some config switch boolean "useConcurrentSessionMap" which if true uses a ConcurrentHashMap and if false uses the synchronized map. Then the user can choose which to use based on their number of expected connections and available RAM. This can default to false to preserve current behaviour.


                        +1

                        • 9. Re: Combined codec and packet
                          timfox

                          For now I have replaced the default implementation with our own implementation based on a ConcurrentHashMap and this has alleviated the point of contention.