[
https://issues.apache.org/jira/browse/CASSANDRA-6694?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13977774#comment-13977774
]
Aleksey Yeschenko commented on CASSANDRA-6694:
----------------------------------------------
h3. Benedict’s original branch
ABTC.ColumnUpdater#apply() calls update.reconcile(existing) and skips
localCopy() if reconciled == existing. This means that we should optimise all
reconcile() implementations to prioritise the argument cell in case of ties for
optimal savings (and ties happen often enough, from retries and whatnot +
potentially counter updates if we decide to do that thing when batch commit log
is enabled). Currently we do the opposite. Would be easiest to simply swap the
call to existing.reconcile(update).
getAllocator() doesn’t belong to SecondaryIndex, API-wise. CFS#logFlush() and
CFS.FLCF#run() should just use SecondaryIndexManager#getIndexesNotBackedByCfs()
and get their allocators directly instead of using SIM#getIndexes() and
checking for null.
Composite/CellName/CellNameType/etc#copy() all now have an extra CFMetaData
argument, while only NativeCell really uses it. Can we isolate its usage to a
NativeCell-specific methods and leave the rest alone?
At least NativeCell#cql3ColumnName() can throw NPE when calling
metadata.getColumnDefinition(buffer).name. Just because it’s SIMPLE_SPARSE
doesn’t mean all the column names are predefined - it’s legal to insert
non-predefined cells w/ default_validator validator via Thrift/CQL2.
NativeCell#copy(), COMPOUND_SPARSE branch - there is no way a compound sparse
comparator and cfType = Super can coexist. Supers are all compound dense.
Generally, NativeCell methods seem to assume a bit too much about the sizes and
about what can and what can’t be present/absent. You can even guarantee
presence of a ColumnIdentifier for COMPOUND_SPARSE, and yet NativeCell#copy()
would throw an AssertionError is that’s the case. And CFMetaData is mutable,
too, and it is possible to remove a column via ALTER TABLE at any time.
I’m not comfortable +1-ing it until Sylvain has a look at at least these bits
(just the NativeCell methods).
Allocator hierarchy is confusing - I won’t claim having understood it entirely,
as are the names there. ‘Data’ prefix in DataAllocator is absolutely
meaningless in the context. Maybe MemtableAllocator would be more meaningful?
Don’t have suggestions for the rest of the names and for making that hierarchy
more straightforward, but I can live with it as it is.
I very much dislike the Impl thing though. This is an uncomfortable step back
in Cell* hierarchy readability. Basic things like using IDEA’s Find Usages on
Cell.Impl#localCopy() not showing Counter/Expiring/Deleted counterparts’ usage
are annoying. This is my largest, and, really the only fundamental issue with
the branch. Other than that, and too many assumptions in certain NativeCell
methods, I’m okay with the branch.
Overall it looks reasonable, and is actually less invasive than I was afraid it
would be.
Nits: AbstractMemory formatting is all messed up.
h3. Pavel’s refactoring branch
Doesn’t build (although trivial-ish to make it build) and is incomplete (as
expected), and that does complicate judging the ugliness of the result.
Same issues and potential issues in AbstractNativeCells methods as in
NativeCell methods in the other branch.
Can’t form an opinion on Pavel’s Allocator/Pool approach, because it’s not here
yet, and I’m not sure I got it right from just reading the comments.
This *Cell hierarchy, though, I feel a lot more comfortable with.
I feel strongly that we should borrow the Impl-less Cell hierarchy from this
branch, if nothing else (and there isn’t much else yet) - this is my biggest
issue with the original.
As for the rest of it - the time is running low, we have to ship 2.1
eventually. Any chance you could flesh it out in the next few days, maybe until
Monday, Pavel? If not, I’m not sure if we should block beta2 further :\
> Slightly More Off-Heap Memtables
> --------------------------------
>
> Key: CASSANDRA-6694
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6694
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Benedict
> Labels: performance
> Fix For: 2.1 beta2
>
>
> The Off Heap memtables introduced in CASSANDRA-6689 don't go far enough, as
> the on-heap overhead is still very large. It should not be tremendously
> difficult to extend these changes so that we allocate entire Cells off-heap,
> instead of multiple BBs per Cell (with all their associated overhead).
> The goal (if possible) is to reach an overhead of 16-bytes per Cell (plus 4-6
> bytes per cell on average for the btree overhead, for a total overhead of
> around 20-22 bytes). This translates to 8-byte object overhead, 4-byte
> address (we will do alignment tricks like the VM to allow us to address a
> reasonably large memory space, although this trick is unlikely to last us
> forever, at which point we will have to bite the bullet and accept a 24-byte
> per cell overhead), and 4-byte object reference for maintaining our internal
> list of allocations, which is unfortunately necessary since we cannot safely
> (and cheaply) walk the object graph we allocate otherwise, which is necessary
> for (allocation-) compaction and pointer rewriting.
> The ugliest thing here is going to be implementing the various CellName
> instances so that they may be backed by native memory OR heap memory.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
