HBASE-11774 Avoid allocating unnecessary tag iterators

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java

@@ -686,6 +686,10 @@ public class AccessControlLists {
 
    public static List<Permission> getCellPermissionsForUser(User user, Cell cell)
        throws IOException {
+     // Save an object allocation where we can
+     if (cell.getTagsLength() == 0) {
+       return null;
+     }
      List<Permission> results = Lists.newArrayList();
      Iterator<Tag> tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
         cell.getTagsLength());

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -758,11 +758,14 @@ public class AccessController extends BaseMasterAndRegionObserver
     for (Map.Entry<byte[], List<Cell>> e: familyMap.entrySet()) {
       List<Cell> newCells = Lists.newArrayList();
       for (Cell cell: e.getValue()) {
+        // Prepend the supplied perms in a new ACL tag to an update list of tags for the cell
         List<Tag> tags = Lists.newArrayList(new Tag(AccessControlLists.ACL_TAG_TYPE, perms));
-        Iterator<Tag> tagIterator = CellUtil.tagsIterator(cell.getTagsArray(),
+        if (cell.getTagsLength() > 0) {
+          Iterator<Tag> tagIterator = CellUtil.tagsIterator(cell.getTagsArray(),
             cell.getTagsOffset(), cell.getTagsLength());
-        while (tagIterator.hasNext()) {
+          while (tagIterator.hasNext()) {
-          tags.add(tagIterator.next());
+            tags.add(tagIterator.next());
+          }
         }
         // Ensure KeyValue so we can do a scatter gather copy. This is only a win if the
         // incoming cell type is actually KeyValue.
@@ -1694,21 +1697,26 @@ public class AccessController extends BaseMasterAndRegionObserver
     List<Tag> tags = Lists.newArrayList();
     ListMultimap<String,Permission> perms = ArrayListMultimap.create();
     if (oldCell != null) {
-      Iterator<Tag> tagIterator = CellUtil.tagsIterator(oldCell.getTagsArray(),
+      // Save an object allocation where we can
+      if (oldCell.getTagsLength() > 0) {
+        Iterator<Tag> tagIterator = CellUtil.tagsIterator(oldCell.getTagsArray(),
           oldCell.getTagsOffset(), oldCell.getTagsLength());
-      while (tagIterator.hasNext()) {
+        while (tagIterator.hasNext()) {
-        Tag tag = tagIterator.next();
+          Tag tag = tagIterator.next();
-        if (tag.getType() != AccessControlLists.ACL_TAG_TYPE) {
+          if (tag.getType() != AccessControlLists.ACL_TAG_TYPE) {
-          if (LOG.isTraceEnabled()) {
+            // Not an ACL tag, just carry it through
-            LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType() +
+            if (LOG.isTraceEnabled()) {
-              " length " + tag.getTagLength());
+              LOG.trace("Carrying forward tag from " + oldCell + ": type " + tag.getType() +
+                " length " + tag.getTagLength());
+            }
+            tags.add(tag);
+          } else {
+            // Merge the perms from the older ACL into the current permission set
+            ListMultimap<String,Permission> kvPerms = ProtobufUtil.toUsersAndPermissions(
+              AccessControlProtos.UsersAndPermissions.newBuilder().mergeFrom(
+                tag.getBuffer(), tag.getTagOffset(), tag.getTagLength()).build());
+            perms.putAll(kvPerms);
           }
-          tags.add(tag);
-        } else {
-          ListMultimap<String,Permission> kvPerms = ProtobufUtil.toUsersAndPermissions(
-            AccessControlProtos.UsersAndPermissions.newBuilder().mergeFrom(
-              tag.getBuffer(), tag.getTagOffset(), tag.getTagLength()).build());
-          perms.putAll(kvPerms);
         }
       }
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java

@@ -356,11 +356,14 @@ public class TableAuthManager {
     try {
       List<Permission> perms = AccessControlLists.getCellPermissionsForUser(user, cell);
       if (LOG.isTraceEnabled()) {
-        LOG.trace("Perms for user " + user.getShortName() + " in cell " + cell + ": " + perms);
+        LOG.trace("Perms for user " + user.getShortName() + " in cell " + cell + ": " +
+          (perms != null ? perms : ""));
       }
-      for (Permission p: perms) {
+      if (perms != null) {
-        if (p.implies(action)) {
+        for (Permission p: perms) {
-          return true;
+          if (p.implies(action)) {
+            return true;
+          }
         }
       }
     } catch (IOException e) {

hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java

@@ -925,22 +925,26 @@ public class VisibilityController extends BaseMasterAndRegionObserver implements
     if (checkAuths && user != null && user.getShortName() != null) {
       auths = this.visibilityManager.getAuthsAsOrdinals(user.getShortName());
     }
-    // Adding all other tags
+    // Prepend new visibility tags to a new list of tags for the cell
-    Iterator<Tag> tagsItr = CellUtil.tagsIterator(newCell.getTagsArray(), newCell.getTagsOffset(),
-        newCell.getTagsLength());
-    while (tagsItr.hasNext()) {
-      Tag tag = tagsItr.next();
-      if (tag.getType() != VisibilityUtils.VISIBILITY_TAG_TYPE
-          && tag.getType() != VisibilityUtils.VISIBILITY_EXP_SERIALIZATION_TAG_TYPE) {
-        tags.add(tag);
-      }
-    }
     try {
       tags.addAll(createVisibilityTags(cellVisibility.getExpression(), true, auths,
-          user.getShortName()));
+        user.getShortName()));
     } catch (ParseException e) {
       throw new IOException(e);
     }
+    // Save an object allocation where we can
+    if (newCell.getTagsLength() > 0) {
+      // Carry forward all other tags
+      Iterator<Tag> tagsItr = CellUtil.tagsIterator(newCell.getTagsArray(), newCell.getTagsOffset(),
+        newCell.getTagsLength());
+      while (tagsItr.hasNext()) {
+        Tag tag = tagsItr.next();
+        if (tag.getType() != VisibilityUtils.VISIBILITY_TAG_TYPE
+            && tag.getType() != VisibilityUtils.VISIBILITY_EXP_SERIALIZATION_TAG_TYPE) {
+          tags.add(tag);
+        }
+      }
+    }
 
     // We need to create another KV, unfortunately, because the current new KV
     // has no space for tags

hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityLabelFilter.java

@@ -80,39 +80,42 @@ class VisibilityLabelFilter extends FilterBase {
       return ReturnCode.SKIP;
     }
 
-    Iterator<Tag> tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
-        cell.getTagsLength());
     boolean visibilityTagPresent = false;
-    while (tagsItr.hasNext()) {
+    // Save an object allocation where we can
-      boolean includeKV = true;
+    if (cell.getTagsLength() > 0) {
-      Tag tag = tagsItr.next();
+      Iterator<Tag> tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
-      if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) {
+        cell.getTagsLength());
-        visibilityTagPresent = true;
+      while (tagsItr.hasNext()) {
-        int offset = tag.getTagOffset();
+        boolean includeKV = true;
-        int endOffset = offset + tag.getTagLength();
+        Tag tag = tagsItr.next();
-        while (offset < endOffset) {
+        if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) {
-          Pair<Integer, Integer> result = StreamUtils.readRawVarint32(tag.getBuffer(), offset);
+          visibilityTagPresent = true;
-          int currLabelOrdinal = result.getFirst();
+          int offset = tag.getTagOffset();
-          if (currLabelOrdinal < 0) {
+          int endOffset = offset + tag.getTagLength();
-            // check for the absence of this label in the Scan Auth labels
+          while (offset < endOffset) {
-            // ie. to check BitSet corresponding bit is 0
+            Pair<Integer, Integer> result = StreamUtils.readRawVarint32(tag.getBuffer(), offset);
-            int temp = -currLabelOrdinal;
+            int currLabelOrdinal = result.getFirst();
-            if (this.authLabels.get(temp)) {
+            if (currLabelOrdinal < 0) {
-              includeKV = false;
+              // check for the absence of this label in the Scan Auth labels
-              break;
+              // ie. to check BitSet corresponding bit is 0
-            }
+              int temp = -currLabelOrdinal;
-          } else {
+              if (this.authLabels.get(temp)) {
-            if (!this.authLabels.get(currLabelOrdinal)) {
+                includeKV = false;
-              includeKV = false;
+                break;
-              break;
+              }
+            } else {
+              if (!this.authLabels.get(currLabelOrdinal)) {
+                includeKV = false;
+                break;
+              }
             }
+            offset += result.getSecond();
+          }
+          if (includeKV) {
+            // We got one visibility expression getting evaluated to true. Good to include this KV in
+            // the result then.
+            return ReturnCode.INCLUDE;
           }
-          offset += result.getSecond();
-        }
-        if (includeKV) {
-          // We got one visibility expression getting evaluated to true. Good to include this KV in
-          // the result then.
-          return ReturnCode.INCLUDE;
         }
       }
     }

hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityUtils.java

@@ -176,6 +176,9 @@ public class VisibilityUtils {
    */
   public static boolean getVisibilityTags(Cell cell, List<Tag> tags) {
     boolean sortedOrder = false;
+    if (cell.getTagsLength() == 0) {
+      return sortedOrder;
+    }
     Iterator<Tag> tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
         cell.getTagsLength());
     while (tagsIterator.hasNext()) {
@@ -200,6 +203,9 @@ public class VisibilityUtils {
    * @return true if found, false if not found
    */
   public static boolean isVisibilityTagsPresent(Cell cell) {
+    if (cell.getTagsLength() == 0) {
+      return false;
+    }
     Iterator<Tag> tagsIterator = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
         cell.getTagsLength());
     while (tagsIterator.hasNext()) {
@@ -270,9 +276,12 @@ public class VisibilityUtils {
   }
 
   private static List<List<Integer>> sortTagsBasedOnOrdinal(Cell cell) throws IOException {
+    List<List<Integer>> fullTagsList = new ArrayList<List<Integer>>();
+    if (cell.getTagsLength() == 0) {
+      return fullTagsList;
+    }
     Iterator<Tag> tagsItr = CellUtil.tagsIterator(cell.getTagsArray(), cell.getTagsOffset(),
         cell.getTagsLength());
-    List<List<Integer>> fullTagsList = new ArrayList<List<Integer>>();
     while (tagsItr.hasNext()) {
       Tag tag = tagsItr.next();
       if (tag.getType() == VisibilityUtils.VISIBILITY_TAG_TYPE) {