[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Michal Frajt commented on JGRP-806: ----------------------------------- The current Message (1.84) is not using the ConcurrentHashMap for the headers anymore. The CCHM is used for the canonical adrresses which is a class static instance. The message headers are currently handled using the default java HashMap class. It might be a good solution if there are many headers attached to a common message object. As far as I could see (debug) from our first test application there was just one header attached to messages recevied from the defalt channel (multicast-localhost). How many headers are added by the protocol stacks to sent/received messages? The header name/value pair could be just simply stored in an Object array and iterated for every header put and every header get call. The array would be extended when there are more headers than preallocated size (parameter) required. The second issue is with the synchronization object currently used for the headers access. The ReentrantReadWriteLock is pretty expensive class to be created for every message sent or received by the protocol stacks. Is there really a situation that the same message object is modified (putHeader) and processed (getHeader) from multiple threads? The message is either sent (down the stack) or received (up the stack) by a single thread execution. I think that an object monitor should be considered for the headers synchronization . # # EXAMPLE # private final Object headersMonitor = new Object(); private Object[] headers = new Object [DEF_HEADERS << 1]; ... public Header getHeader(String key) { synchronized(this.headersMonitor ) { for(int i = 0; i < header.length; i+=2) { if(this.headers[i] == null) { return null; } else if(this.headers[i].equals(key)) { return (Header) this.header[i + 1]; } } } return null; } ... -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Bela Ban commented on JGRP-806: ------------------------------- Removing the header_lock and *NOT* locking at all yields a perf gain: 2 nodes, 500'000 1K messages, 2 senders. With locks: 30'000 msgs/sec/node, without: 39'000 msgs/sec/node ! We really need to investigate whether we need synchronization around headers. As Michal mentioned, no message should concurrently add and get a header... -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Michal Frajt commented on JGRP-806: ----------------------------------- I am happy to hear that the heavy RRWL is removed from the Message class. Please reconsider again implementing the headers as the simple array object. The cost with the HashMap implementation is the precreation of the Entry objects when the map is initialized. Your initial capacity for the headers is currently 7. It means that the HashMap constructor precreates 7 instances of the HashMap.Entry class. The each Entry class is about 3xref (12 bytes) and one int (4 bytes) plus the Java object itself. There is the object array created for the Entries with the same length (7xref 28bytes). The HashMap has additionaly another refs and ints (~32 bytes). All together ~172 bytes within 9 java objects (another memory overhead). All the memory is required to store around ~2-4 headers for each message. The implementation using the array might take only 2 refs (8 bytes) per header. We might precreate the array for 4 headers => 4*2*4=32bytes and all as one array object only. There is a huge difference for the newgc (many Message instances are created/sent/received within short moment) to get rid of one 32bytes array and to get rid of ~172 bytes split into 9 objects (I suppose that many JGroups applications are using the CMS garbage collector but it should be valid on the default GC as well). The very small overhead you pay for every get/put iteration is returned at the GC time. Next to the memory required to store the headers within the HashMap there is additionall memory required to answer the size method of the Message class. There is a hidden cost of the Map iteration where the HashMap must create a HashMap.EntrySet object for the entrySet() call return. Additionally there must be an iterator object created for every iteration within every size() method call. The total cost of using the map is than increased by another 2 objects with minimum 32 bytes data. Minimum garbage created by HashMap headers for 58.000 messages (sec/node) 58.000 x ((172+32b) as 11 objects) => ~12MB Garbage created by Array headers for 58.000 messages (sec/node) 58.000 x (32 as 1 object) => ~2MB We are able to help you with the implementation of the Message class using the array for headers. There might be only small issue with the getHeaders method which is used in dumpQueue (Util), printMessage(HDRS) and toString() from Event. We might still return a headers map copy for each call (compatibility with existing code) and create a new method getHeader(int index) for simple fast garbage-less iteration (there is already method getNumHeaders available). The three methods using getHeaders should change to the new headers iteration style. # NEW HEADERS ITERATOR EXAMPLE final int headersCount = msg.getNumHeaders(); for(int i = 0; i < headersCount; ++i) { final Header header = msg.getHeader(i); /// ... /// } For us is very important to use API's which do not generate any non-required garbage. Over the time we have moved to the code not using even iterators to iterate over lists/sets/maps to reduce the garbage creation. We are using customized version of the GNU Trove classes for the critical parts of our applications. Looking forward to see new test results with the reimplemented headers! Michal -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Vladimir Blagojevic commented on JGRP-806: ------------------------------------------ Looking at JDK5 source I don't see that HashMap creates Entry objects as you claim Michal. It only creates an array of Entry objects. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Michal Frajt commented on JGRP-806: ----------------------------------- Bela, I just saw MessageStressTest code. You forgot to call the size() method to invoke the creation of the EntrySet object and of the iterator object. It will slow down and add another garbage for the HashMap version test. public long size() { .... header_lock.readLock().lock(); try { for(Iterator it=headers.entrySet().iterator(); it.hasNext();) { entry=(Map.Entry)it.next(); key=(String)entry.getKey(); retval+=key.length() +2; // not the same as writeUTF(), but almost hdr=(Header)entry.getValue(); retval+=(Global.SHORT_SIZE *2); // 2 for magic number, 2 for size (short) retval+=hdr.size(); } } finally { header_lock.readLock().unlock(); } ... } -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Bela Ban resolved JGRP-806. --------------------------- Resolution: Done -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Bela Ban commented on JGRP-806: ------------------------------- #1 Okay, I changed resize() to be unsynchronized. #2 I have resize() double its capacity, but then changed it to linear growth. This is optimized for an average number of 3 headers. I certainly never have more than 10 headers, so I didn't want to double the size #3 The Message.getHeaders() method is not called by any important method other than Event.toString(), which I changed to call Message.printHeaders() Note that you *can* iterate over the array without producing garbage: the getRawData() method of Headers allows you to do this -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira

[ https://jira.jboss.org/jira/browse/JGRP-806?page=com.atlassian.jira.plugi... ] Bela Ban commented on JGRP-806: ------------------------------- Excellent ! I changed the code locally today (no commits): #1 Use of IDs (ints) rather than strings as keys: performance was about the same, but space overhead was bigger #2 Use of intern() with all keys. Performance dropped to 50% ! So it seems that the Headers class is quite efficient ! Would you be able to contribute your changes to MessageStressTest to JGroups ? -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira