[GitHub] [accumulo] belugabehr commented on a change in pull request #1333: Removed LinkedList from CachedBlockQueue in favor of ArrayList

[GitBox <gi...@apache.org>]

belugabehr commented on a change in pull request #1333: Removed LinkedList from CachedBlockQueue in favor of ArrayList
URL: https://github.com/apache/accumulo/pull/1333#discussion_r322436505
 
 

 ##########
 File path: core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/lru/CachedBlockQueue.java
 ##########
 @@ -91,24 +93,19 @@ public void add(CachedBlock cb) {
    * @return list of cached elements in descending order
    */
   public CachedBlock[] get() {
-    LinkedList<CachedBlock> blocks = new LinkedList<>();
-    while (!queue.isEmpty()) {
-      blocks.addFirst(queue.poll());
-    }
-    return blocks.toArray(new CachedBlock[blocks.size()]);
+     CachedBlock[] blocks = queue.toArray(new CachedBlock[0]);
+     Arrays.sort(blocks, Collections.reverseOrder(queue.comparator()));
 
 Review comment:
   ```
   # Run complete. Total time: 00:01:07
   
   Benchmark                        Mode  Cnt      Score     Error  Units
   BenchmarkQueueDrain.clearDrain  thrpt   30  11440.641 ± 211.193  ops/s
   BenchmarkQueueDrain.whileDrain  thrpt   30  11653.781 ± 145.950  ops/s
   ```
   
   So the two implementations are within the margin of error.  What I cannot measure is the cost of creating all of the nodes in the `LinkedList`.  There was no JVM pressure in my perf test so that cost was not included,... v.s. just a single array created and a sort applied.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services