[GitHub] activemq-artemis pull request #1263: ARTEMIS-1156: FIX: Long Autoboxing occu...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
68 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1263: ARTEMIS-1156: FIX: Long Autoboxing occu...

tabish121-2
GitHub user michaelandrepearce opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1263

    ARTEMIS-1156: FIX: Long Autoboxing occurring on Hot Path

    Building on ARTEMIS-905 JCtools ConcurrentMap replacement  first proposed but currently parked by @franz1981, replace the collections with primitive key concurrent collections to avoid auto boxing.
   
    The goal of this is to reduce/remove autoboxing on the hot path.
    We are just adding jctools to the broker (should not be in client dependencies)
    Like wise targeting specific use case with specific implementation rather than a blanket replace all.
   
    We have to create our own primitive long concurrent hash set, based on jctools as jctools currently don't have one.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1156

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1263.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1263
   
----
commit 7f8b0077a6717f023387c1bc42db23229c47a9c6
Author: Michael Andre Pearce <[hidden email]>
Date:   2017-05-10T07:13:24Z

    ARTEMIS-1156: FIX: Long Autoboxing occurring on Hot Path
   
    Building on ARTEMIS-905 JCtools ConcurrentMap replacement  first proposed but currently parked by @franz1981, replace the collections with primitive key concurrent collections to avoid auto boxing.
   
    The goal of this is to reduce/remove autoboxing on the hot path.
    We are just adding jctools to the broker (should not be in client dependencies)
    Like wise targeting specific use case with specific implementation rather than a blanket replace all.
   
    We have to create our own primitive long concurrent hash set, based on jctools as jctools currently don't have one.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    See Jira with profiler output showing long object creation caused by auto boxing [https://issues.apache.org/jira/browse/ARTEMIS-1156](url)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @michaelandrepearce Good work and great findings!
    My 2 cents on it, the new map is great but:
    1) it has a slightly different behaviour about atomic operations (eg [ConcurrentHashMap::compute javadoc](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#compute-K-java.util.function.BiFunction-)) hence please checks if there are callers relying on such behaviours
    2)  it doesn't shrink/grow  like the vanilla ConcurrentHashMap -> when it grows it is not incremental by little chuncks, but could put a lot of pressure on Humongous Allocations hurting the heap allocations of byte[] performed during message translations    
    3) JournalCompactor in JournalImpl::compac is using a keySet from it (producing the unwanted Longs
   
    So it is important to make sure that the common usage do not make them happen or at least , that the benefits are well balanced by these drawbacks
    Anyway, the PR is great and I'm pretty happy about it :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @franz1981
    On 1) compute isn't being used within this class, the methods used are ::get ::put ::contains ::containsKey, indeed this is why i state this is targeted, instead of your original blanket replace all, each case should be evaluated and implemented on merit.
   
    On 2) agreed, in warm up stages we would see this, but once warm and message rate through the broker, it would stabilise its size, this is why we don't see hashmap.entry being created heavily in the profiler once warm.
   
    On 3) good spot. lets address that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @franz1981 just updated to add in the journal compactor.
   
    I ran out of time today, to deploy and run this with a profiler on our servers to verify that this hasn't pushed the auto boxing further down or else where, i will do this tomorrow.
   
    In particular can you look at the way i have handled addAll for the NonBlockingHashSetLong, is there a better way?
   
    Also for the HashMap that had long keys in the journal which is not concurrent, I've used the LongHashMap from netty.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    I want to look into this before merge.  I'm out of my home office now. I will be back in a bit today.
   
   
    Also. I want to spend time on reference counts on messages this next weeks and use pooled buffers on messages as well.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1263: ARTEMIS-1156: FIX: Long Autoboxing occu...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1263#discussion_r115794851
 
    --- Diff: artemis-journal/src/main/java/org/jctools/maps/NonBlockingHashSetLong.java ---
    @@ -0,0 +1,105 @@
    +/*
    + * Licensed under the Apache License, Version 2.0 (the "License");
    + * you may not use this file except in compliance with the License.
    + * You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICLongNSLong-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTILongS OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.jctools.maps;
    +
    +import java.io.Serializable;
    +import java.util.AbstractSet;
    +import java.util.Iterator;
    +
    +/**
    + * A simple wrapper around {@link NonBlockingHashMapLong} making it implement the
    + * {@link java.util.Set} interface.  All operations are Non-Blocking and multi-thread safe.
    + */
    +public class NonBlockingHashSetLong extends AbstractSet<Long> implements Serializable {
    --- End diff --
   
    Isn't this class available on any jctools package?
    Can't we just reuse the library instead of copy & paste?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    No. Unfortunately not, there is an int version and a hashset for objects (but that will cause autoboxing)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    it is largely based on the NonBlockingHashSet that internally uses NonBlockingHashMap taking object keys, but replaced the internal map with their NonBlockingHashMapLong and added the primitive methods.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @michaelandrepearce can you fix the checkstyle


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @michaelandrepearce Oh.. never mind.. I can ammend during checkout.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    Yeah still got a few bits to clean up. Also I need to run it on server under load I ran out of time today. But review comments would be good


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    Well there's another fix I have locally I need to commit to fix the distribution as it's missing the dependency in the zip/tar go. But on the move atm


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    dep.xml updated and check style fixed.
   
    have managed to get results back from testing, allocation of longs is greatly reduced (oddly theres one autobox in JournalTransaction, ill have to check that again, but now at least significant sector is just the byte[] from the unpooled buffer.
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    <img width="1001" alt="screen shot 2017-05-10 at 19 04 05" src="https://cloud.githubusercontent.com/assets/1387822/25913734/b9930f5e-35b3-11e7-84af-a4073e0643c3.png">
    <img width="1001" alt="screen shot 2017-05-10 at 19 03 02" src="https://cloud.githubusercontent.com/assets/1387822/25913736/b999c646-35b3-11e7-928f-8a64c9e70e64.png">



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    from 127mb prior and now just 11mb. :), and i think we can remove that also :) might have a look what the Integer is upto, in the allocations , it looks like it auto boxing also.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @michaelandrepearce are you using replication? FileWrapperJournal used on replication has a HashMap<Long>


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    ok so the int, is due to connection.getID return Object, for the two implementations netty it is int, and InVM its a String UUID. I would suggest the getID becomes an int on the interfaces, and where the InVM uses currently UUID to regenerate a unique id, as its within a VM we can use a Sequence to generate a unique int.
   
    This is a bit more invasive, so i won't to that in this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1263: ARTEMIS-1156: FIX: Long Autoboxing occurring o...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1263
 
    @michaelandrepearce +1 on changing the connection.getID() to a generated sequence.
   
    It's a bit invasive though as ProtocolClientConnectionManager and other places will use the ID to remove failed connections... etc...
   
    https://issues.apache.org/jira/browse/ARTEMIS-1158


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1263: ARTEMIS-1156: FIX: Long Autoboxing occu...

tabish121-2
In reply to this post by tabish121-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1263#discussion_r115828438
 
    --- Diff: artemis-journal/src/main/java/org/jctools/maps/NonBlockingHashSetLong.java ---
    @@ -0,0 +1,105 @@
    +/*
    + * Licensed under the Apache License, Version 2.0 (the "License");
    + * you may not use this file except in compliance with the License.
    + * You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICLongNSLong-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTILongS OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.jctools.maps;
    +
    +import java.io.Serializable;
    +import java.util.AbstractSet;
    +import java.util.Iterator;
    +
    +/**
    + * A simple wrapper around {@link NonBlockingHashMapLong} making it implement the
    + * {@link java.util.Set} interface.  All operations are Non-Blocking and multi-thread safe.
    + */
    +public class NonBlockingHashSetLong extends AbstractSet<Long> implements Serializable {
    --- End diff --
   
    @michaelandrepearce would be ok to move this under utils, under an artemis package? just in case.. I think that's more clear, especially with osgi usage.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
1234