[ 
https://issues.apache.org/jira/browse/ACCUMULO-4468?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15510743#comment-15510743
 ] 

Josh Elser commented on ACCUMULO-4468:
--------------------------------------

{quote}
bq.    Do you have other examples of where this might be used in a tight loop?

I think there are lots of other examples in Accumulo itself; for example, in 
iterators.system.DeletingIterator and iterators.user.TransformingIterator, this 
method is called in a loop. In our use case, we're doing a similar thing: 
building a larger object out of multiple rows, by finding groups of rows which 
are equal under ROW_COLFAM. When each object is built from only a few rows, the 
CF equality comparison returns false pretty often (which is to be expected), 
but only after comparing row IDs, which are always the same in practice.
{quote}

Perhaps my concern didn't come across. From my perspective: I am concerned with 
a performance change that makes one case better. We need to understand if the 
case you outlined is "the norm" or "the exception". I was hoping you had 
context on this.

{quote}
bq.  How did you test this? What types of numbers did you see?

I haven't been able to install it on a cluster to test. The test suite does 
pass with this patch applied. I think it's a minor change; in the "rows are 
equal" case the same amount of work is done as with the existing code, although 
the parts are accessed in the opposite order. They're still compared 
mostly-in-order, as isEqual does, but the comment in that function was 
inspiration to try reversing the comparison order.

Aside from performance, the code seems cleaner to me: there's no more 
repetition of e.g. the check of row equality. The bytecode (with Oracle javac 
1.8.0_92) is substantially smaller: 389 bytes versus 167.
{quote}

At risk of being anti-social, I am -1 on any change for performance without 
numbers coming with it. There are many great tools out there to benchmark 
changes and don't necessarily require use of a cluster (it might actually be 
harder to test on a cluster). 
[JMH|http://openjdk.java.net/projects/code-tools/jmh/] and [Google 
Caliper|https://github.com/google/caliper] are two good tools for 
micro-benchmarking.

> accumulo.core.data.Key.equals(Key, PartialKey) improvement
> ----------------------------------------------------------
>
>                 Key: ACCUMULO-4468
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-4468
>             Project: Accumulo
>          Issue Type: Improvement
>          Components: core
>    Affects Versions: 1.8.0
>            Reporter: Will Murnane
>            Priority: Trivial
>              Labels: newbie, performance
>         Attachments: key_comparison.patch
>
>
> In the Key.equals(Key, PartialKey) overload, the current method compares 
> starting at the beginning of the key, and works its way toward the end. This 
> functions correctly, of course, but one of the typical uses of this method is 
> to compare adjacent rows to break them into larger chunks. For example, 
> accumulo.core.iterators.Combiner repeatedly calls this method with subsequent 
> pairs of keys.
> I have a patch which reverses the comparison order. That is, if the method is 
> called with ROW_COLFAM_COLQUAL_COLVIS, it will compare visibility, cq, cf, 
> and finally row. This (marginally) improves the speed of comparisons in the 
> relatively common case where only the last part is changing, with less 
> complex code.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to