Accumulo
  1. Accumulo
  2. ACCUMULO-2039

Authorizations.getAuthorizationsBB is inefficient

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:

      Description

      Authorizations.getAuthorizations was reworked for 1.6.0 to build an immutable list of copies of contained authorizations. The Authorizations.getAuthorizationsBB method then takes that list and makes another immutable list of copies. The latter method could be made much more efficient by eliminating the extra copies and (possibly) list construction.

        Activity

        Hide
        Vikram Srivastava added a comment -

        Attached patch.

        Show
        Vikram Srivastava added a comment - Attached patch.
        Hide
        Bill Havanki added a comment -

        The patch looks fine.

        One question for other committers: The patch eliminates a public method in ByteBufferUtil, since it was only used by the code being changed and could be refactored away. Is that elimination OK?

        Show
        Bill Havanki added a comment - The patch looks fine. One question for other committers: The patch eliminates a public method in ByteBufferUtil, since it was only used by the code being changed and could be refactored away. Is that elimination OK?
        Hide
        Keith Turner added a comment -

        Removing that public method is ok, its not part of the Accumulo API

        Show
        Keith Turner added a comment - Removing that public method is ok, its not part of the Accumulo API
        Hide
        Sean Busbey added a comment -

        ByteBufferUtil is not in our public API, so ti should be fine. We've been known to leak that abstraction, so I'd recommend checking the Accumulo contrib repos to make sure something else isn't impacted.

        I noticed this defensively copies the actual byte arrays. Do we have some quick way to check for a performance regression on this? I'm not sure how extensively we use getAuthorizationsBB, so I don't know how much overhead we're talking about adding.

        An alternative would be to use the byte arrays in place but call asReadOnlyBuffer to make sure the generated BB's can't be modified. This won't work if whomever is using the BB needs to grab the underlying byte array, because read only buffer's don't allow access to them (I think Avro's serialization code does this).

        Show
        Sean Busbey added a comment - ByteBufferUtil is not in our public API, so ti should be fine. We've been known to leak that abstraction, so I'd recommend checking the Accumulo contrib repos to make sure something else isn't impacted. I noticed this defensively copies the actual byte arrays. Do we have some quick way to check for a performance regression on this? I'm not sure how extensively we use getAuthorizationsBB, so I don't know how much overhead we're talking about adding. An alternative would be to use the byte arrays in place but call asReadOnlyBuffer to make sure the generated BB's can't be modified. This won't work if whomever is using the BB needs to grab the underlying byte array, because read only buffer's don't allow access to them (I think Avro's serialization code does this).
        Hide
        Keith Turner added a comment -

        The thrift code may try to get the array, so the read only byte buffer may not work.

        Show
        Keith Turner added a comment - The thrift code may try to get the array, so the read only byte buffer may not work.
        Hide
        Vikram Srivastava added a comment -

        Sean Busbey I don't think there is any performance regression because of copying the byte array since the previous code was also doing the same thing by calling getAuthorizations() which also makes a copy of the byte array.

        Also, note that if I didn't do the defensive copy, AuthorizationsTest.testReadOnlyByteBuffer was breaking. So please let me know if I am wrong, but I believe we do want to protect against the underlying byte arrays getting modified.

        I'm also checking the accumulo-contrib repos to see if the removed method was getting called anywhere.

        Show
        Vikram Srivastava added a comment - Sean Busbey I don't think there is any performance regression because of copying the byte array since the previous code was also doing the same thing by calling getAuthorizations() which also makes a copy of the byte array. Also, note that if I didn't do the defensive copy, AuthorizationsTest.testReadOnlyByteBuffer was breaking. So please let me know if I am wrong, but I believe we do want to protect against the underlying byte arrays getting modified. I'm also checking the accumulo-contrib repos to see if the removed method was getting called anywhere.
        Hide
        Vikram Srivastava added a comment -

        Update - I couldn't find any references to ByteBufferUtil in contrib projects.

        Show
        Vikram Srivastava added a comment - Update - I couldn't find any references to ByteBufferUtil in contrib projects.
        Hide
        Sean Busbey added a comment -

        +1 looks good. (at some point in the future I'd like confirmation on the thrift code needing to reach in, since we could save the copy)

        Show
        Sean Busbey added a comment - +1 looks good. (at some point in the future I'd like confirmation on the thrift code needing to reach in, since we could save the copy)
        Hide
        ASF subversion and git services added a comment -

        Commit 9a34281d394199eb0dc156d196946b76be47ac43 in branch refs/heads/1.6.0-SNAPSHOT from Vikram Srivastava
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=9a34281 ]

        ACCUMULO-2039 Make Authorizations.getAuthorizationsBB efficient

        Signed-off-by: Bill Havanki <bhavanki@cloudera.com>

        Show
        ASF subversion and git services added a comment - Commit 9a34281d394199eb0dc156d196946b76be47ac43 in branch refs/heads/1.6.0-SNAPSHOT from Vikram Srivastava [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=9a34281 ] ACCUMULO-2039 Make Authorizations.getAuthorizationsBB efficient Signed-off-by: Bill Havanki <bhavanki@cloudera.com>
        Hide
        ASF subversion and git services added a comment -

        Commit 9a34281d394199eb0dc156d196946b76be47ac43 in branch refs/heads/master from Vikram Srivastava
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=9a34281 ]

        ACCUMULO-2039 Make Authorizations.getAuthorizationsBB efficient

        Signed-off-by: Bill Havanki <bhavanki@cloudera.com>

        Show
        ASF subversion and git services added a comment - Commit 9a34281d394199eb0dc156d196946b76be47ac43 in branch refs/heads/master from Vikram Srivastava [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=9a34281 ] ACCUMULO-2039 Make Authorizations.getAuthorizationsBB efficient Signed-off-by: Bill Havanki <bhavanki@cloudera.com>
        Hide
        ASF subversion and git services added a comment -

        Commit c11e41cbf5d732c574c1aab96d1df56458c4240d in branch refs/heads/master from Bill Havanki
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=c11e41c ]

        ACCUMULO-2039 Merge branch '1.6.0-SNAPSHOT'

        Show
        ASF subversion and git services added a comment - Commit c11e41cbf5d732c574c1aab96d1df56458c4240d in branch refs/heads/master from Bill Havanki [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=c11e41c ] ACCUMULO-2039 Merge branch '1.6.0-SNAPSHOT'
        Hide
        Bill Havanki added a comment -

        Committed. Thanks Vikram!

        Show
        Bill Havanki added a comment - Committed. Thanks Vikram!
        Hide
        Mike Drob added a comment -

        Bill Havanki - it's not strictly necessary to put the JIRA on the merge commit message, especially when it's a clean merge.

        Show
        Mike Drob added a comment - Bill Havanki - it's not strictly necessary to put the JIRA on the merge commit message, especially when it's a clean merge.
        Hide
        Bill Havanki added a comment -

        Thanks for the tip Mike!

        Vikram: Please let me know how you want your information to appear on the Accumulo contributors' page: http://accumulo.apache.org/people.html

        Show
        Bill Havanki added a comment - Thanks for the tip Mike! Vikram: Please let me know how you want your information to appear on the Accumulo contributors' page: http://accumulo.apache.org/people.html

          People

          • Assignee:
            Vikram Srivastava
            Reporter:
            Bill Havanki
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development