[ https://issues.apache.org/jira/browse/HBASE-18555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16123212#comment-16123212 ]
Xiang Li commented on HBASE-18555: ---------------------------------- Hi Mike, Regarding bq. Your modification to getCellList makes it look almost identical to the get/null-check/put pattern though. What am I missing? yes, the patch updates the logic of Mutation#getCellList(family) to be get/null-check/put pattern. Please allow me to summarize the code path in different scenarios * Without the patch ** When cell list for a family exists: get list from map -> null check -> update list -> {color:#d04437}put list to map (could be removed){color} ** When cell list for a family does not exist: get list from map -> null check -> allocate list -> {color:#59afe1}update list -> put list to map{color} * With the patch ** When cell list for a family exists: {color:#14892c}get list from map -> null check -> update list {color} ** When cell list for a family does not exist: get list from map -> null check -> allocate list -> {color:#59afe1}put list to map -> update list{color} The patch targets to remove the redundant Map#put() when cell list for a family already exists (remove the red to make it as green) It also changes the logic when the cell list for a family does not exit, by moving Map#put() to be in front of List#add(), in blue, but does not do any harm. > Remove redundant familyMap.put() from Put#addColumn() and addImmutable() > ------------------------------------------------------------------------ > > Key: HBASE-18555 > URL: https://issues.apache.org/jira/browse/HBASE-18555 > Project: HBase > Issue Type: Improvement > Components: Client > Reporter: Xiang Li > Assignee: Xiang Li > Priority: Minor > Attachments: HBASE-18555.master.000.patch > > > In Put#addColumn() and addImmutable(), after getting the cell list of the > given family and add the cell into the list, the code puts (key=family, > value=list) into familyMap. > In addColumn(), it is like > {code} > List<Cell> list = getCellList(family); > KeyValue kv = createPutKeyValue(family, qualifier, ts, value); > list.add(kv); > familyMap.put(CellUtil.cloneFamily(kv), list); // <-- here > return this; > {code} > In addImmutable(), it is like > {code} > List<Cell> list = getCellList(family); > KeyValue kv = createPutKeyValue(family, qualifier, ts, value, tag); > list.add(kv); > familyMap.put(family, list); // <-- here > return this; > {code} > I think those put() for Map only take effect when getCellList(family) returns > a new allocated ArrayList. When the list for a family already exist, put() > for Map will update the value to the reference of the list, but actually, the > reference of the list is not changed. > Those put() do not do any harm in terms of the correctness when they are > here. But it could be removed to improve the performance. familyMap searches > for key and set the new value and return the old value. Those operation take > some time but actually are not needed. > The put() could be moved into Mutation#getCellList(family) -- This message was sent by Atlassian JIRA (v6.4.14#64029)