[jira] [Comment Edited] (CASSANDRA-15393) Add byte array backed cells

["Marcus Eriksson (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17189237#comment-17189237 ] 

Marcus Eriksson edited comment on CASSANDRA-15393 at 9/2/20, 2:10 PM:
----------------------------------------------------------------------

First pass done, LGTM in general, I'll try to get a second pass done this week.

*Issues*

{{CounterContext.java}}
 * {{hasLegacyShards(..)}} {{totalCount}} looks wrong, should probably be {{int totalCount = (accessor.size(context) - headerLength(context, accessor)) / STEP_LENGTH;}}

{{ByteBufferAccessor.java}}
 * {{digest(..)}} needs to add {{value.position}} to {{offset}} edit: maybe not, guess value.position should always be 0 now

{{DynamicCompositeType.java}}
 * in {{validateComparator(..)}}, adds {{1}} to {{offset}} after reading a {{short}}

*Nits*
 General stuff
 * Slightly inconsistent use of {{TypeSizes.INT_SIZE}} / {{TypeSize.sizeof(int)}} / hard coded {{4}}, I'd probably prefer being explicit and use X_SIZE, mostly since we represent shorts etc in ints
 * A few places added the {{left}} / {{right}} / {{accessorL}} / {{accessorR}} parameters, but then have {{offset1/offset2/size1/size2}} - maybe rename these to {{offsetL/offsetR}} etc

{{CQL3Type.java}}
 * {{generateXCQLLiteral}}: no need to return the offset
 * unused import ByteBufferUtil

{{AbstractClusteringPrefix.java}}
 * maybe bring up {{size()}} and {{get(i)}} here (as {{getRawValues().length}} and {{getRawValues()[i]}}), and only leave them overridden in {{NativeClustering}}

{{ClusteringPrefix.java}}
 * {{isBottom}} & {{isTop}} can use {{size()}} instead of {{getRawValues().length}}

{{ArrayClusteringBound.java}} + {{ArrayClusteringBoundary.java}} + {{BufferClusteringBound.java}} + {{BufferClusteringBoundary.java}}
 * unsharedHeapSizeExcludingData unused

{{AbstractType.java}}
 * {{readArray}} unused

{{ByteBufferAccessor.java}}
 * in {{compare(..)}} - instead of casting to a {{ByteBuffer}} maybe use {{accessor.toBuffer(right)}} (same in {{ByteArrayAccessor}})

{{DynamicCompositeType.java}}
 * no need to {{getComparatorSize}} in {{getComparator(int i, VL left, ValueAccessor<VL> accessorL, VR right, ValueAccessor<VR> accessorR, int offset1, int offset2)}}

{{MapType.java}}
 * don't use {{V}} as the type parameter in {{referencesUserType}} - quite confusing as it clashes with the {{MapType<K, V>}} type param

{{ValueAccessor.java}}
 * why have {{compare}} / {{compareUnsigned}} with the same parameters?


was (Author: krummas):
First pass done, LGTM in general, I'll try to get a second pass done this week.

*Issues*

{{CounterContext.java}}
 * {{hasLegacyShards(..)}} {{totalCount}} looks wrong, should probably be {{int totalCount = (accessor.size(context) - headerLength(context, accessor)) / STEP_LENGTH;}}

{{ByteBufferAccessor.java}}
 * {{digest(..)}} needs to add {{value.position}} to {{offset}}

{{DynamicCompositeType.java}}
 * in {{validateComparator(..)}}, adds {{1}} to {{offset}} after reading a {{short}}

*Nits*
 General stuff
 * Slightly inconsistent use of {{TypeSizes.INT_SIZE}} / {{TypeSize.sizeof(int)}} / hard coded {{4}}, I'd probably prefer being explicit and use X_SIZE, mostly since we represent shorts etc in ints
 * A few places added the {{left}} / {{right}} / {{accessorL}} / {{accessorR}} parameters, but then have {{offset1/offset2/size1/size2}} - maybe rename these to {{offsetL/offsetR}} etc

{{CQL3Type.java}}
 * {{generateXCQLLiteral}}: no need to return the offset
 * unused import ByteBufferUtil

{{AbstractClusteringPrefix.java}}
 * maybe bring up {{size()}} and {{get(i)}} here (as {{getRawValues().length}} and {{getRawValues()[i]}}), and only leave them overridden in {{NativeClustering}}

{{ClusteringPrefix.java}}
 * {{isBottom}} & {{isTop}} can use {{size()}} instead of {{getRawValues().length}}

{{ArrayClusteringBound.java}} + {{ArrayClusteringBoundary.java}} + {{BufferClusteringBound.java}} + {{BufferClusteringBoundary.java}}
 * unsharedHeapSizeExcludingData unused

{{AbstractType.java}}
 * {{readArray}} unused

{{ByteBufferAccessor.java}}
 * in {{compare(..)}} - instead of casting to a {{ByteBuffer}} maybe use {{accessor.toBuffer(right)}} (same in {{ByteArrayAccessor}})

{{DynamicCompositeType.java}}
 * no need to {{getComparatorSize}} in {{getComparator(int i, VL left, ValueAccessor<VL> accessorL, VR right, ValueAccessor<VR> accessorR, int offset1, int offset2)}}

{{MapType.java}}
 * don't use {{V}} as the type parameter in {{referencesUserType}} - quite confusing as it clashes with the {{MapType<K, V>}} type param

{{ValueAccessor.java}}
 * why have {{compare}} / {{compareUnsigned}} with the same parameters?

> Add byte array backed cells
> ---------------------------
>
>                 Key: CASSANDRA-15393
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-15393
>             Project: Cassandra
>          Issue Type: Sub-task
>          Components: Local/Compaction
>            Reporter: Blake Eggleston
>            Assignee: Blake Eggleston
>            Priority: Normal
>             Fix For: 4.0-beta
>
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> We currently materialize all values as on heap byte buffers. Byte buffers have a fairly high overhead given how frequently they’re used, and on the compaction and local read path we don’t do anything that needs them. Use of byte buffer methods only happens on the coordinator. Using cells that are backed by byte arrays instead in these situations reduces compaction and read garbage up to 22% in many cases.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org