[ https://issues.apache.org/jira/browse/HBASE-14921?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15212838#comment-15212838 ]
stack commented on HBASE-14921: ------------------------------- Reviewed again after another read of the attached diagram. Does the 'CellBlock' Map have to be a Concurrent Map? This is a readonly datastructure once made? No need of the 'Concurrent'? If so, you made CellSet (or Eshcar) made CellSet which implements NaviableSet. Here we are slinging Cells and implement NavigableMap. Call it CellMap rather than CellBock? I was trying to come up with a name that made reference to how it is actually implemented -- e.g. like ConcurrentSkipListMap -- which would seem to say all it CellArrayMap rather than CellBlock but when I see CellBlockObjectArray later.. may be CellBinSearchMap.... It is odd that comparator is passed on construction and when we call createCellBlocks. Should constructor be protected so folks can't construct one w/o there being data in it? Should we save the comparator when createCellBlocks rather than pass on construction? Then we have to make sure the comparators agree? Or do we? Seems like you need it on construction so can propagate instances with the createCellBlocks call. Seems like allowing passing different comparators would make for trouble so don't allow passing comparator on createCellBlocks? Why name the method getCellFromIndex(int i)? Should it be get(i) because we are getting the ith Cell in Map? (The comment says that this is what it is doing) It'd be cool to compare a CSLM to one of these CAMs at different scales to see difference in perf in a test bench. For CellBlockObjectArray, it should be called CellArrayMap? What does "CellBlockObjectArray is a simple array of Cells allocated using JVM." mean? All our stuff is on a JVM (smile) Below should be final Cell[] block; Is the below total Cels? private int numOfCellsInsideChunk; It looks like it is Cells per chunk? This is constant for all Chunks in a CellBlockSerialized? (BTW CellBlockSerialized could be ChunkCellMap or CellMLABMap This seems to be the 'key', or 'index key' size, not bytes in a cell?: private static final int BYTES_IN_CELL = 3*(Integer.SIZE / Byte.SIZE); // each Cell requires 3 integers I love the way this works. Very nice. Simple. Only bit is how you ensure all Chunks have same amount of Cells? Remove this stuff rather than just comment it out: //c = (chunkPool != null) ? chunkPool.getChunk() : new Chunk(chunkSize, 5); //14921 197 The 14921 everywhere should be stripped. Its HBASE-14921? Is this a hardcoding? 201 c = new Chunk(chunkSize, 5); The Chunk Pool needs to be on for this all to work? Is that in this patch? For... * Only used in testing we usually mark with @VisibleForTestiing annotation. Patch looks great. > Memory optimizations > -------------------- > > Key: HBASE-14921 > URL: https://issues.apache.org/jira/browse/HBASE-14921 > Project: HBase > Issue Type: Sub-task > Affects Versions: 2.0.0 > Reporter: Eshcar Hillel > Assignee: Anastasia Braginsky > Attachments: CellBlocksSegmentInMemStore.pdf, > CellBlocksSegmentinthecontextofMemStore(1).pdf, HBASE-14921-V01.patch, > HBASE-14921-V02.patch > > > Memory optimizations including compressed format representation and offheap > allocations -- This message was sent by Atlassian JIRA (v6.3.4#6332)