You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by wy96f <gi...@git.apache.org> on 2018/02/05 11:07:40 UTC

[GitHub] activemq-artemis pull request #1851: fix npe bug while getting element in th...

GitHub user wy96f opened a pull request:

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

    fix npe bug while getting element in the process of rehash

    Hi, we found a bug in ConcurrentLongHashMap, as follows:
    
    12:32:31,000 WARN  [org.apache.activemq.artemis.core.replication.ReplicationEndpoint] 295: java.lang.ArrayIndexOutOfBoundsException: 295
            at org.apache.activemq.artemis.utils.collections.ConcurrentLongHashMap$Section.get(ConcurrentLongHashMap.java:230) [artemis-commons-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.utils.collections.ConcurrentLongHashMap.get(ConcurrentLongHashMap.java:117) [artemis-commons-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.journal.impl.JournalImpl.appendCommitRecord(JournalImpl.java:1185) [artemis-journal-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.journal.impl.JournalBase.appendCommitRecord(JournalBase.java:69) [artemis-journal-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.journal.impl.JournalImpl.appendCommitRecord(JournalImpl.java:91) [artemis-journal-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.replication.ReplicationEndpoint.handleCommitRollback(ReplicationEndpoint.java:635) [artemis-server-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.replication.ReplicationEndpoint.handlePacket(ReplicationEndpoint.java:191) [artemis-server-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.protocol.core.impl.ChannelImpl.handlePacket(ChannelImpl.java:638) [artemis-core-client-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.protocol.core.impl.RemotingConnectionImpl.doBufferReceived(RemotingConnectionImpl.java:392) [artemis-core-client-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.protocol.core.impl.RemotingConnectionImpl.bufferReceived(RemotingConnectionImpl.java:374) [artemis-core-client-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.client.impl.ClientSessionFactoryImpl$DelegatingBufferHandler.bufferReceived(ClientSessionFactoryImpl.java:1144) [artemis-core-client-2.4.0.jar:2.4.0]
            at org.apache.activemq.artemis.core.remoting.impl.netty.ActiveMQChannelHandler.channelRead(ActiveMQChannelHandler.java:68) [artemis-core-client-2.4.0.jar:2.4.0]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:310) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:297) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:413) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:265) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1359) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:935) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:797) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:404) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:304) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:858) [netty-all-4.1.16.Final.jar:4.1.16.Final]
            at java.lang.Thread.run(Thread.java:745) [rt.jar:1.8.0_121]
    
    The get thread saw doubled capacity first and at the moment values had not been assigned new values, so npe occurred. We should assign capacity after the keys and values. The volatile modifier would be needed to generate store barrier to make sure keys/values data are visible to other threads before the capacity is.

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

    $ git pull https://github.com/wy96f/activemq-artemis concurrent_hash_long_map

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

    https://github.com/apache/activemq-artemis/pull/1851.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 #1851
    
----
commit ca6548d1739d4dba1e7114742ddf06a2a84aae3a
Author: yang wei <wy...@...>
Date:   2018-02-05T10:12:18Z

    fix npe bug while getting element in the process of rehash

----


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @michaelandrepearce hi, i don't get what you mean the test. Why do we need a suit test? And where should the test be put to stop from running by CI buid?


---

[GitHub] activemq-artemis pull request #1851: ARTEMIS-1664 fix npe bug while getting ...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1851#discussion_r170185326
  
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMap.java ---
    @@ -457,11 +459,11 @@ private void rehash() {
                 }
              }
     
    -         capacity = newCapacity;
              keys = newKeys;
              values = newValues;
              usedBuckets = size;
    -         resizeThreshold = (int) (capacity * MapFillFactor);
    +         capacityUpdater.lazySet(this, newCapacity);
    +         resizeThreshold = (int) (newCapacity * MapFillFactor);
    --- End diff --
    
    `capacityUpdater.lazySet` is store-releasing `keys` , `values`, `usedBuckets`, but not `resizeThreshold`: that means that the `resizeThreshold` store could be happen before the update of the `capacity`, is it correct?



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @franz1981  Hi, I'm a little puzzled about padding technique. In your example, i know that stampedLock fileds are in different cache lines for different sections but fields like capacity, size, etc can be in the same cache line with the next section's stampedLock fields. So the thread will clash with other threads and cause cache invalidations, is it right?


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @michaelandrepearce Yes agree :+1: 
    I've performed a couple of benchs and that's what I'm getting (reusing this benchmark: ):
    ```
    master:
    ConcurrentLongHashMapThroughput.randomGetPutRemove           50       100000  thrpt   60  26906004.642 ± 1014602.753  ops/s
    
    plain load size:
    ConcurrentLongHashMapThroughput.randomGetPutRemove           50       100000  thrpt   60  29136460.289 ± 604244.629  ops/s
    ```
    IMO it worths to be pushed as a separate PR as you suggested


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f 
    > In your example
    
    My example was just to show the method used to perform a padding with an existing abstract class, has not values from the perf purposes: sorry, my mistake to not have specified it at first :P
    
    > i know that stampedLock fileds are in different cache lines for different sections
    
    For any of these reasonings consider 2 facts:
    1. false sharing must not  be considered between cache lines but on prefetching size basis (ie 128 bytes == 2 cache lines)
    2. the GC could arbitrarly choose to scatter Section instances on the heap in an order different from Section[] ie it is a GC implementation detail, although it is mostly true in this case
    
    Hence, it would be great to protect each Section fields from any surrounding part of the heap that could invalidate its fields cache lines for any reasons: it could be another Section (what is likely to happen)  or any "living" instance the GC has chosen to move near to them.
    
    In our case that could happen but probably it is more necessary to be "fixed" in the original class of Bookkeeper, that's more general purpose.
    
    
    
    
    
    
    
    
    
    
    



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @franz1981 1. I agree 2. Adding unsafe  is not trivial and lazySet is enough 3. size() is seldomly called, isn't it? 4. We can add @sun.misc.Contended in Section class, right?


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @franz1981 lazySet is used to update capacity. Please review it :)


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    Seems first time travis build failed for unrelated reason second attempt 
    https://github.com/apache/activemq-artemis/pull/1892
    
    is fine.


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    Oops, i deleted the branch :( It seems not possible to reopen. I'll create a new one.


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    I would be careful of over optimizing this. They let reason for the use of this class was to reduce the long autoboxing. It’s used throughout the code base where concurrent hash map used to be. 
    
    It comes also from Bookkeeper where it’s heavily used.
    
    I think we have bigger fish to fry in artemis than over optimizing this.


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @michaelandrepearce Agree about aggressive optimisations but not on volatile operation's: isn't the volatile read that cost but the volatile store that include an hidden cost a of a full barrier on x86 , to be paid   on hot paths and even if in contended. Dropoing it using lazySet or a plain store (for size) from the functional point of view it really can't introduce any new bugs, but is clear that is an implementation error from who has written the original class.


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    Please rebase this and push -f so the new Travis CI build for the PR will run. Thanks!


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f could you rebase (it seems clebert merged my branch (which had your original code) i used for checking that the build passed in the night)
    
    Also once rebased could you apply the same change to the Set implementation also?
    
    As for the test having a longer running test i think is fine, just i think if it goes in as a full test suite test e.g. it wont run every PR.


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    There are tests that run as part of PR and then separately there are a fuller test suite. Reason for this is the larger test suite takes much longer to complete, as such it’s run periodically or if larger changes are made. See the hackers guide for more info 


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f 
    > Adding unsafe is not trivial and lazySet is enough
    
    Probably we have already access to it thanks to `io.netty.util.internal.shaded.org.jctools.util.UnsafeAccess` but it could be tricky to win the karaf validation if used, so I agree :+1: 
    > size() is seldomly called, isn't it?
    
    The load yes, but the store is on hot paths (eg put): dropping the cost of a volatile store/full fence could be very welcome on writer side (ig less latencies spikes while writing on the journal).
    
    > We can add @sun.misc.Contended in Section class, right
    
    It could be tricky like using `sun.misc.Unsafe` directly in the code, due to the proprietary `sun.misc.*`, there are unglier but nonetheless effective ways to do it with plain Java too.
    For example this is padding the lock fields from the one of the section using the same padding size of `@Contended` ie 128 bytes:
    ```
       abstract static class SectionL0Pad extends StampedLock {
    
          long p01, p02, p03, p04, p05, p06, p07;
          long p10, p11, p12, p13;
       }
    
       // A section is a portion of the hash map that is covered by a single
       @SuppressWarnings("serial")
       private static final class Section<V> extends SectionL0Pad {
    ```
    Using [jol](http://openjdk.java.net/projects/code-tools/jol/) it shows:
    ```
    org.apache.activemq.artemis.utils.collections.ConcurrentLongHashMap$Section object internals:
     OFFSET  SIZE                                                       TYPE DESCRIPTION                               VALUE
          0    12                                                            (object header)                           N/A
         12     4                                                        int StampedLock.readerOverflow                N/A
         16     8                                                       long StampedLock.state                         N/A
         24     4               java.util.concurrent.locks.StampedLock.WNode StampedLock.whead                         N/A
         28     4               java.util.concurrent.locks.StampedLock.WNode StampedLock.wtail                         N/A
         32     4        java.util.concurrent.locks.StampedLock.ReadLockView StampedLock.readLockView                  N/A
         36     4       java.util.concurrent.locks.StampedLock.WriteLockView StampedLock.writeLockView                 N/A
         40     4   java.util.concurrent.locks.StampedLock.ReadWriteLockView StampedLock.readWriteLockView             N/A
         44     4                                                            (alignment/padding gap)                  
         48     8                                                       long SectionL0Pad.p01                          N/A
         56     8                                                       long SectionL0Pad.p02                          N/A
         64     8                                                       long SectionL0Pad.p03                          N/A
         72     8                                                       long SectionL0Pad.p04                          N/A
         80     8                                                       long SectionL0Pad.p05                          N/A
         88     8                                                       long SectionL0Pad.p06                          N/A
         96     8                                                       long SectionL0Pad.p07                          N/A
        104     8                                                       long SectionL0Pad.p10                          N/A
        112     8                                                       long SectionL0Pad.p11                          N/A
        120     8                                                       long SectionL0Pad.p12                          N/A
        128     8                                                       long SectionL0Pad.p13                          N/A
        136     4                                                        int Section.capacity                          N/A
        140     4                                                        int Section.size                              N/A
        144     4                                                        int Section.usedBuckets                       N/A
        148     4                                                        int Section.resizeThreshold                   N/A
        152     4                                                     long[] Section.keys                              N/A
        156     4                                         java.lang.Object[] Section.values                            N/A
    Instance size: 160 bytes
    ```
    While the original version layout were:
    ```
    org.apache.activemq.artemis.utils.collections.ConcurrentLongHashMap$Section object internals:
     OFFSET  SIZE                                                       TYPE DESCRIPTION                               VALUE
          0    12                                                            (object header)                           N/A
         12     4                                                        int StampedLock.readerOverflow                N/A
         16     8                                                       long StampedLock.state                         N/A
         24     4               java.util.concurrent.locks.StampedLock.WNode StampedLock.whead                         N/A
         28     4               java.util.concurrent.locks.StampedLock.WNode StampedLock.wtail                         N/A
         32     4        java.util.concurrent.locks.StampedLock.ReadLockView StampedLock.readLockView                  N/A
         36     4       java.util.concurrent.locks.StampedLock.WriteLockView StampedLock.writeLockView                 N/A
         40     4   java.util.concurrent.locks.StampedLock.ReadWriteLockView StampedLock.readWriteLockView             N/A
         44     4                                                        int Section.capacity                          N/A
         48     4                                                        int Section.size                              N/A
         52     4                                                        int Section.usedBuckets                       N/A
         56     4                                                        int Section.resizeThreshold                   N/A
         60     4                                                     long[] Section.keys                              N/A
         64     4                                         java.lang.Object[] Section.values                            N/A
         68     4                                                            (loss due to the next object alignment)
    Instance size: 72 bytes
    ```
    Anyway before doing anything would be necessary to measure any benefits by adding something like that.
    Our current use case for this collection is on the journal ie "mostly" single writer: the original collection was meant to be used on multiple writers scenarios and probably it could benefit from this technique.



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    Sorry for the late reply. Using lazySet to update capacity filed is a good point. As for the test, I modified test as follows:
    
    @Test
       public void concurrentInsertionsAndReads() throws Throwable {
          ExecutorService executor = Executors.newCachedThreadPool();
    
          for (int x = 0; x < 1000; x++) {
             ConcurrentLongHashMap<String> map = new ConcurrentLongHashMap<>(1, 1);
    
             final int nThreads = 16;
             final int N = 100_000;
             String value = "v";
    
             List<Future<?>> futures = new ArrayList<>();
             for (int i = 0; i < nThreads; i++) {
                futures.add(executor.submit(() -> {
                   Random random = new Random();
    
                   for (int j = 0; j < N; j++) {
                      long key = random.nextLong();
                      // Ensure keys are uniques
                      //key -= key % (threadIdx + 1);
    
                      map.get(key);
                   }
                }));
    
                final int threadIdx = i;
    
                futures.add(executor.submit(() -> {
                   Random random = new Random();
    
                   for (int j = 0; j < N; j++) {
                      long key = random.nextLong();
                      // Ensure keys are uniques
                      key -= key % (threadIdx + 1);
    
                      map.put(key, value);
                   }
                }));
    
             }
    
             for (Future<?> future : futures) {
                future.get();
             }
    
             assertEquals(map.size(), N * nThreads);
          }
    
          executor.shutdown();
       }
    Most likely, npe would occur. The test may cost a long time to trigger the bug. Is it ok to run the test for such a long time in the junit test?


---

[GitHub] activemq-artemis pull request #1851: ARTEMIS-1664 fix npe bug while getting ...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f closed the pull request at:

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


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @jbertram i have taken @wy96f just rebased and done a PR so travis CI will build see https://github.com/apache/activemq-artemis/pull/1891
    
    If that passes we can merge this PR (and ill close my branch of their work)


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f If you want to take a look/review the code I have pushed https://github.com/apache/activemq-artemis/pull/1895 to improve the volatile usage witht the results I've attached in the above comments


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f Np you can pick up the master and push a new PR with the new changes that are more improvements/gardening than fixes.
    IMO would be interesting to:
    - drop the (completly useless) benchmark on ConcurrentLongHashMapTest
    - investigate re lazySet on capacity (probably just an Unsafe::storeFence would enough)
    - investigate re lazySet on size or just making it a plain field and do something like:
    ```
       public int size() {
          int size = 0;
          for (Section<V> s : sections) {
             //read acquire the lock state and any written size
             s.tryOptimisticRead();
             size += s.size;
          }
          return size;
       }
    ```
    - [advanced] investigate about padding to avoid false sharing between Section fields



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f Well done, please address the changes I've proposed re lazySet and test :+1: 


---

[GitHub] activemq-artemis pull request #1851: ARTEMIS-1664 fix npe bug while getting ...

Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1851#discussion_r166307381
  
    --- Diff: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMap.java ---
    @@ -457,10 +457,10 @@ private void rehash() {
                 }
              }
     
    -         capacity = newCapacity;
              keys = newKeys;
              values = newValues;
              usedBuckets = size;
    +         capacity = newCapacity;
    --- End diff --
    
    Please check if is possible to use a lazySet too (maybe with an `AtomicFieldUpdater` or similar) to write release the keys/values in order to avoid a full barrier


---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @wy96f could you update your commit message with that.



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    @franz, agreed volatile hit is on the write wy96f only changed the capacity to be volatile and changed the order they’re set to fix a bug. this only changes when the map is resized not every put. (Note size was already volatile)
    
    As such it fixes the bug. If you , I or wy96f wants to further optimize that then that’s fine but I think that should be separate case to the functional bug. And obviously should be on its own merits. 
    



---

[GitHub] activemq-artemis issue #1851: ARTEMIS-1664 fix npe bug while getting element...

Posted by wy96f <gi...@git.apache.org>.
Github user wy96f commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1851
  
    Hi, franz1981. Thanks for the review. The jira issue is ARTEMIS-1664.


---