[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

clebertsuconic-3
GitHub user franz1981 opened a pull request:

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

    ARTEMIS-1476 HdrHistogram support on verbose SyncCalculation

    It includes:
    - improved documentation of SyncCalculation::syncTest
    - HdrHistogram "write" latencies supports with verbose

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

    $ git pull https://github.com/franz1981/activemq-artemis sync_calc_histo

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

    https://github.com/apache/activemq-artemis/pull/1605.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 #1605
   
----
commit c7eb0b907f1ded94b8c2a16c1cf95ffcf02ad795
Author: Francesco Nigro <[hidden email]>
Date:   2017-10-23T16:12:33Z

    ARTEMIS-1476 HdrHistogram support on verbose SyncCalculation

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1605
 
    One example of the possible usage of the results using http://hdrhistogram.github.io/HdrHistogram/plotFiles.html is:
    ![histogram](https://user-images.githubusercontent.com/13125299/31900747-d2c36250-b81f-11e7-8254-457de0102a25.png)



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146353744
 
    --- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/util/SyncCalculation.java ---
    @@ -69,74 +171,55 @@ public static long syncTest(File datafolder,
     
              file.close();
     
    -         long[] result = new long[tries];
    +         final long[] elapsedMillis = new long[tries];
     
    -         byte[] block = new byte[blockSize];
    -
    -         for (int i = 0; i < block.length; i++) {
    -            block[i] = (byte) 't';
    -         }
    -
    -         ByteBuffer bufferBlock = factory.newBuffer(blockSize);
    -         bufferBlock.put(block);
    -         bufferBlock.position(0);
    -
    -         final ReusableLatch latch = new ReusableLatch(0);
    -
    -         IOCallback callback = new IOCallback() {
    -            @Override
    -            public void done() {
    -               latch.countDown();
    -            }
    -
    -            @Override
    -            public void onError(int errorCode, String errorMessage) {
    -
    -            }
    -         };
    -
    -         DecimalFormat dcformat = new DecimalFormat("###.##");
    +         final DecimalFormat dcformat = new DecimalFormat("###.##");
              for (int ntry = 0; ntry < tries; ntry++) {
     
    +            //perform a gc on each test iteration to help cleanup of callbacks/garbage
    +            System.gc();
    --- End diff --
   
    I wouldn't do this.
   
    The previous version was pretty much garbage free... if you must do this, I wouldn't merge the commit at all.
   
   
    Besides... System.gc() will start a gc in parallel, and you will be already measuring the next write iteration.. which will affect the performance.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146354093
 
    --- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/util/SyncCalculation.java ---
    @@ -193,18 +291,29 @@ private static SequentialFileFactory newFactory(File datafolder, boolean datasyn
     
              case NIO:
                 factory = new NIOSequentialFileFactory(datafolder, 1).setDatasync(datasync);
    -            ((NIOSequentialFileFactory) factory).disableBufferReuse();
    +            if (bufferReuse) {
    +               ((NIOSequentialFileFactory) factory).enableBufferReuse();
    --- End diff --
   
    This is also wrong... you are changing the semantic of this test...
   
   
    We are measuring how much write the system can do... Using the buffer will defeat the purpose of the test...
   
   
    Besides you are using the timeouts from where? this is not the intention of this calculation.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

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

    https://github.com/apache/activemq-artemis/pull/1605
 
    You told me you would just add verbose output here.. not change semantics of the test for no reason.
   
   
    There was a lot of testing around this... changing the semantics like this out the blank without the proper testing won't help much.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

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

    https://github.com/apache/activemq-artemis/pull/1605
 
    This adds complexity that we don't need.. you're even requiring GCs to perform now.. the previous version wouldn't generate any garbage during the measurements (just simple buffer.write all the time.. always the same buffer).


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146450033
 
    --- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/util/SyncCalculation.java ---
    @@ -69,74 +171,55 @@ public static long syncTest(File datafolder,
     
              file.close();
     
    -         long[] result = new long[tries];
    +         final long[] elapsedMillis = new long[tries];
     
    -         byte[] block = new byte[blockSize];
    -
    -         for (int i = 0; i < block.length; i++) {
    -            block[i] = (byte) 't';
    -         }
    -
    -         ByteBuffer bufferBlock = factory.newBuffer(blockSize);
    -         bufferBlock.put(block);
    -         bufferBlock.position(0);
    -
    -         final ReusableLatch latch = new ReusableLatch(0);
    -
    -         IOCallback callback = new IOCallback() {
    -            @Override
    -            public void done() {
    -               latch.countDown();
    -            }
    -
    -            @Override
    -            public void onError(int errorCode, String errorMessage) {
    -
    -            }
    -         };
    -
    -         DecimalFormat dcformat = new DecimalFormat("###.##");
    +         final DecimalFormat dcformat = new DecimalFormat("###.##");
              for (int ntry = 0; ntry < tries; ntry++) {
     
    +            //perform a gc on each test iteration to help cleanup of callbacks/garbage
    +            System.gc();
    --- End diff --
   
    Good point, I'm addressing all these things, but I need feedback on that too :)
    I see 2 options here:
    1) preallocate and cache the `blocks` IO callbacks (but could be a large number too) in order to be garbage free again
    2) let the IO callbacks to size the (young) GC regions in the first test but uses a better way (ie more reliable) to be sure GC won't affect the measurements
   
    I'm pushing the second solution right now, using the code used by the official microbenchmark tool of the OpenJDK JMH (http://hg.openjdk.java.net/code-tools/jmh/file/e96cad1fc480/jmh-core/src/main/java/org/openjdk/jmh/runner/BaseRunner.java#l309) to perform reliable GC, but feel free to give me feedbacks and suggest me to try the first one too, although I'm not 100% sure could be a better way (cachin a large number of instances in not optimal!!).



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146450916
 
    --- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/util/SyncCalculation.java ---
    @@ -69,74 +171,55 @@ public static long syncTest(File datafolder,
     
              file.close();
     
    -         long[] result = new long[tries];
    +         final long[] elapsedMillis = new long[tries];
     
    -         byte[] block = new byte[blockSize];
    -
    -         for (int i = 0; i < block.length; i++) {
    -            block[i] = (byte) 't';
    -         }
    -
    -         ByteBuffer bufferBlock = factory.newBuffer(blockSize);
    -         bufferBlock.put(block);
    -         bufferBlock.position(0);
    -
    -         final ReusableLatch latch = new ReusableLatch(0);
    -
    -         IOCallback callback = new IOCallback() {
    -            @Override
    -            public void done() {
    -               latch.countDown();
    -            }
    -
    -            @Override
    -            public void onError(int errorCode, String errorMessage) {
    -
    -            }
    -         };
    -
    -         DecimalFormat dcformat = new DecimalFormat("###.##");
    +         final DecimalFormat dcformat = new DecimalFormat("###.##");
              for (int ntry = 0; ntry < tries; ntry++) {
     
    +            //perform a gc on each test iteration to help cleanup of callbacks/garbage
    +            System.gc();
    --- End diff --
   
    We can always write the same thing. Why cache large numbers of Anything?  Just write and send the same thing over and over.
   
   
    This thing is settled.  Not under production. We should leave this one as is IMO.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146450966
 
    --- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/util/SyncCalculation.java ---
    @@ -69,74 +171,55 @@ public static long syncTest(File datafolder,
     
              file.close();
     
    -         long[] result = new long[tries];
    +         final long[] elapsedMillis = new long[tries];
     
    -         byte[] block = new byte[blockSize];
    -
    -         for (int i = 0; i < block.length; i++) {
    -            block[i] = (byte) 't';
    -         }
    -
    -         ByteBuffer bufferBlock = factory.newBuffer(blockSize);
    -         bufferBlock.put(block);
    -         bufferBlock.position(0);
    -
    -         final ReusableLatch latch = new ReusableLatch(0);
    -
    -         IOCallback callback = new IOCallback() {
    -            @Override
    -            public void done() {
    -               latch.countDown();
    -            }
    -
    -            @Override
    -            public void onError(int errorCode, String errorMessage) {
    -
    -            }
    -         };
    -
    -         DecimalFormat dcformat = new DecimalFormat("###.##");
    +         final DecimalFormat dcformat = new DecimalFormat("###.##");
              for (int ntry = 0; ntry < tries; ntry++) {
     
    +            //perform a gc on each test iteration to help cleanup of callbacks/garbage
    +            System.gc();
    --- End diff --
   
    Not under production I mean. This is only used once to create the server.  Just leave it be.  


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1605
 
    @clebertsuconic
    Good points! I've addressed all these things (with few other fixes), but I need feedbacks on the result.
    I see 2 options here:
        1) preallocate and cache the IO callbacks (but could be a large number too) in order to be garbage free again
        2) let the IO callbacks to size the (young) GC regions in the first tests but uses a better way (ie more reliable) to be sure GC won't affect the measurements
   
    I've pushed the second solution right now, using a method to perform reliable GCs (+ await them to happen) used in the official microbenchmark tool of the OpenJDK JMH too (http://hg.openjdk.java.net/code-tools/jmh/file/e96cad1fc480/jmh-core/src/main/java/org/openjdk/jmh/runner/BaseRunner.java#l309).
   
    Feel free to give me feedbacks on it and if we need something similar in other parts (a more reliable way to perform/await GCs to happen) I can put it in our utlity classes too.
    @michaelandrepearce @lulf I know you're pretty skilled in benchmarking too so please share your thoughts/opinions: are super wellcome!!!!



---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1605
 
    @clebertsuconic As a note: now without `--verbose` it is garbage free as in the original version :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

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

    https://github.com/apache/activemq-artemis/pull/1605
 
    The issue is on the performWrite.  You are creating buffers and callbacks for each write.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1605
 
    In the last version I've pushed I'm reusing the same buffer


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1605: ARTEMIS-1476 HdrHistogram support on verbose S...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146535926
 
    --- Diff: artemis-cli/pom.xml ---
    @@ -83,6 +83,10 @@
              <version>${commons.lang.version}</version>
           </dependency>
           <dependency>
    +         <groupId>org.hdrhistogram</groupId>
    --- End diff --
   
    Whats the license of this?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146536414
 
    --- Diff: pom.xml ---
    @@ -625,6 +626,15 @@
                 <!-- License: Apache 2.0 -->
              </dependency>
     
    +         <!-- needed by SyncCalculation -->
    +         <!-- https://mvnrepository.com/artifact/org.hdrhistogram/HdrHistogram -->
    +         <dependency>
    +            <groupId>org.hdrhistogram</groupId>
    +            <artifactId>HdrHistogram</artifactId>
    +            <version>${org.hdrhistogram.version}</version>
    +            <!-- License: Apache 2.0 -->
    --- End diff --
   
    I don't believe it is Apache license.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146536958
 
    --- Diff: pom.xml ---
    @@ -625,6 +626,15 @@
                 <!-- License: Apache 2.0 -->
              </dependency>
     
    +         <!-- needed by SyncCalculation -->
    +         <!-- https://mvnrepository.com/artifact/org.hdrhistogram/HdrHistogram -->
    +         <dependency>
    +            <groupId>org.hdrhistogram</groupId>
    +            <artifactId>HdrHistogram</artifactId>
    +            <version>${org.hdrhistogram.version}</version>
    +            <!-- License: Apache 2.0 -->
    --- End diff --
   
    Yep: super good point!! It is not obvious at all...I'm checking it!!!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146537426
 
    --- Diff: pom.xml ---
    @@ -625,6 +626,15 @@
                 <!-- License: Apache 2.0 -->
              </dependency>
     
    +         <!-- needed by SyncCalculation -->
    +         <!-- https://mvnrepository.com/artifact/org.hdrhistogram/HdrHistogram -->
    +         <dependency>
    +            <groupId>org.hdrhistogram</groupId>
    +            <artifactId>HdrHistogram</artifactId>
    +            <version>${org.hdrhistogram.version}</version>
    +            <!-- License: Apache 2.0 -->
    --- End diff --
   
    This one: https://github.com/HdrHistogram/HdrHistogram/blob/master/LICENSE.txt
    Argh!!!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146537959
 
    --- Diff: pom.xml ---
    @@ -625,6 +626,15 @@
                 <!-- License: Apache 2.0 -->
              </dependency>
     
    +         <!-- needed by SyncCalculation -->
    +         <!-- https://mvnrepository.com/artifact/org.hdrhistogram/HdrHistogram -->
    +         <dependency>
    +            <groupId>org.hdrhistogram</groupId>
    +            <artifactId>HdrHistogram</artifactId>
    +            <version>${org.hdrhistogram.version}</version>
    +            <!-- License: Apache 2.0 -->
    --- End diff --
   
    Seems BSD 2 mmmmm


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1605: ARTEMIS-1476 HdrHistogram support on ve...

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

    https://github.com/apache/activemq-artemis/pull/1605#discussion_r146539097
 
    --- Diff: pom.xml ---
    @@ -625,6 +626,15 @@
                 <!-- License: Apache 2.0 -->
              </dependency>
     
    +         <!-- needed by SyncCalculation -->
    +         <!-- https://mvnrepository.com/artifact/org.hdrhistogram/HdrHistogram -->
    +         <dependency>
    +            <groupId>org.hdrhistogram</groupId>
    +            <artifactId>HdrHistogram</artifactId>
    +            <version>${org.hdrhistogram.version}</version>
    +            <!-- License: Apache 2.0 -->
    --- End diff --
   
    Seems compatible...@clebert wdyt?


---
123