[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17193628#comment-17193628 ] Marcus Eriksson commented on CASSANDRA-15393: - +1 assuming clean test run and Calebs last nits fixed > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17193067#comment-17193067
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
Read through the last raft of commits, and things are looking pretty good. Some
closing nits...
- {{ByteArrayAccessor#toString(byte[], Charset)}} doesn't look like it needs
to have {{CharacterCodingException}} in its {{throws}} clause, although if you
want to keep it for uniformity with the interface, I suppose that's fine.
- Many of the tests in {{ByteArrayUtilTest}} seem to fail because we removed
{{ByteArrayUtil#ensureCapacity()}}, which produces a more detailed error
message. Not sure if we want to keep that around or leave it out (performance?)
and adjust the tests to expect the simpler error.
- {{ValueAccessor#createArray()}} and {{compare()}} have a dangling JavaDoc
{{@return}}.
- {{ValueAccessor#write()}} typo: "...into the a ByteBuffer"
- {{ValueAccessor#slice()}} typo: "...@prarm offset..."
- {{ValueAccessor#compareByteArrayTo()}} and {{compareByteBufferTo()}} typo:
"...\{@parame }..."
- {{TupleType#split()}} still seems like it would be a pretty easy place to
revert to {{ByteBuffer}} only to we don't have to worry about having introduced
any offset-related bugs? (i.e. Literally its only usage is an overload that
passes the {{ByteBufferAccessor}} singleton.)
- The declaration of {{Unfiltered.Kind}} doesn't need a semicolon following it.
- Might still be nice to make a note in the code around why {{Unfiltered}} has
to extend {{Clusterable}} without a type bound.
- In {{ByteArrayUtil#writeWithShortLength()}}, in the {{assert}}, {{0 <=
length}} is always true.
- Are the FIXMEs in {{DurationSerializer}} still live?
bq. I also didn't convert the randomized tests to QT.
That's fine w/ me. It's a pretty low-value conversion.
The above notwithstanding, +1 from me overall at this point, assuming clean
test runs, etc.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17192414#comment-17192414 ] Blake Eggleston commented on CASSANDRA-15393: - Pushed up a new branch [here|https://github.com/bdeggleston/cassandra/tree/15393-rebase-09-02], this was recently rebased onto trunk and I think I've addressed all review feedback, including reverting several buffer to accessor conversions, with the exceptions below: * I didn't convert ConterContext.clearAllLocal to byte[] only because all the tests are written with byte buffers. * For ModificationStatement and *CBuilder, I started attempting to unwind this and remembered why I'd done it in the first place. Basically, the places where you can be explicit (ie: ModificationStatement) are tangentially linked to the places where we can't be (ie: Cell) through classes used in the coordinator and replica read paths. Things like slices and filters and stuff. Using across the board avoids having to deal with places where you need to figure out how to use Comparator> from ModificaionStatement with Clustering from a Row. * The lack of a native accessor is no worse than the current state of things, and would be straightforward to add in the future. * I didn't remove version from the collection serializers * I also didn't convert the randomized tests to QT. I'm not opposed to the idea, but I don't have the time to devote to the conversion right now. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17189237#comment-17189237
]
Marcus Eriksson commented on CASSANDRA-15393:
-
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 accessorL, VR right, ValueAccessor 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}} 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17186875#comment-17186875
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
[~bdeggleston] There are the [items
above|https://issues.apache.org/jira/browse/CASSANDRA-15393?focusedCommentId=17185489=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17185489]
to resolve, of course, but here's my full feedback on the latest raft of
commits...
*Complexity*
- The semantics of {{ValueAccessor#size()}} are a little odd that it always
indicates the length of the array in the byte array case, but the number of
bytes left in a byte buffer. Some of the usages are just checking if the value
is empty, which makes me wonder if having an isEmpty() get rid of some of the
awkwardness. On the other hand, most usages occur right before we read.
- There are some switch statements we might be able to avoid in the two
accessor implementations if we encode the {{copy()}} and {{compare}} strategies
in {{BackingKind}}.
- How valuable is {{ValueAccessor.copy()}} when we can make calls like
{{srcAccessor.copyTo(src, srcOffset, dst, dstAccessor, dstOffset, size)}} call
directly from {{CounterContext}}? Same for {{compareUnsigned()}}. Want to make
sure we aren't cluttering the accessor interface.
- This is really an extension of the conversation above, but I'm worried that a
handful of places where we only use {{ByteBuffer}} being changed to use the
more abstract value/accessor combination are not only problematic for
readability, but are adding more surface area for testing.
{{TupleType#split()}}, for instance, might not be covered too well, but we
could probably avoid additional tests and get to the immediate goal faster here
if we simply stay in {{ByteBuffer}} space and avoid the new offset logic in the
first place. Other examples of this could be
{{CounterContext#clearAllLocal()}}, {{ClusteringPrefix.Deserializer}} (byte
array only), and {{Clustering.Serializer#deserialize()}}.
*Formatting*
- Horizontal alignment in {{ClusteringIndexNamesFilter#forPaging()}} on the
ternary.
- Might have to line wrap some parameter lists in
{{RangeTombstoneBoundaryMarker}}.
*Documentation*
- Brief JavaDoc for {{ByteBufferAccessor#copyTo()}} might be helpful.
- JavaDoc for {{Unfiltered#clustering()}}, perhaps just to explain that we
couldn't just use the {{clustering()}} signature in {{Clusterable}}?
- What level of JavaDoc do we want for {{ValueAccessor}} and {{ObjectFactory}}
at the interface and method level? Certainly a lot of these methods are pretty
straightforward, but it's also a pretty critical public interface.
*Miscellaneous*
- A number of places that call {{serializeBuffer()}} with both arguments could
just call the version with one argument.
- There are some cases, like in {{CassandraIndex.Indexer#indexCell()}}, where
it would be nice to bind {{Clustering}} and {{Cell}} to the same type, but I
couldn't find a clean way to do it.
- {{BufferClusteringBoundOrBoundary#exclusiveCloseInclusiveOpen()}} looks
unused.
- {{CollectionSerializer}} has quite a few methods that don't do anything w/
{{version}}. (Perhaps there will be another serialized form later, so I won't
argue hard to remove.)
- {{SinglePartitionSliceBuilder#sliceBuilder}} looks like it can be {{final}}.
- The {{Relationship}} enum in {{CounterContext}} doesn't need to have a
{{static}} modifier?
- {{ByteArrayUtil#writeWithLength()}} is unused.
- What about renaming {{ComparatorSet}} to {{ValueComparators}} and the field
in {{AbstractType}} to {{comparators}} (which I think we do in one other place
already)?
- What are the {{// FIXME: value input buffer}} comments in
{{DurationSerializer}} about?
- {{putByte()}}, {{putDouble()}}, {{putFloat()}}, {{putLong()}} are neither
used nor tested. (The underlying {{ByteArrayUtil}} stuff lingering even if
unused seems less an issue.)
- {{import java.nio.ByteBuffer}} use unused in {{RangeTombstoneMarker}}.
*Testing*
- Did we want to make {{CQL3TypeLiteralTest}} deterministic with the fixed zero
seed?
- {{ByteArrayUtilTest}} is empty?
- {{logger}} is unused in {{ReadCommandTest}}
- The new randomized tests here (ex. {{ValueAccessorTest}}) might be able to
reuse some of the infrastructure [~dcapwell] created in CASSANDRA-16064
(generating random primitives, etc.) It might also just be nice to make QT the
standard way we do randomization for true unit tests (free seed printing, etc.)
I'm not making a hard suggestion that we re-write, since the new tests look
pretty straightforward as is.
> 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:
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17186177#comment-17186177 ] Josh McKenzie commented on CASSANDRA-15393: --- I think it was the "perf improvements are allowed up until release as per the agreed upon release lifecycle" so no reason to block beta1 on it. We can discuss on ML. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17186021#comment-17186021 ] Blake Eggleston commented on CASSANDRA-15393: - That's reasonable, I'll ping the dev list. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17186017#comment-17186017 ] Michael Semb Wever commented on CASSANDRA-15393: [~bdeggleston], a patch with 208 files and ~3.5k LOC, [~snazy] expressing concern, and not seeing any comment of why this was placed in 4.0-beta in May. That should be enough that our normal community response is to double check with a broader base. Maybe [~JoshuaMcKenzie] you can provide support and the reasoning why this was, and should be, in 4.0-beta? > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17186013#comment-17186013 ] Blake Eggleston commented on CASSANDRA-15393: - [~mck], this ticket was been triaged to 4.0-beta back in May. Performance improvements aren't restricted by the feature freeze. Do you have concerns about this ticket that aren't related to the feature freeze? > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185997#comment-17185997
]
Michael Semb Wever commented on CASSANDRA-15393:
{quote}Is there's a big difference between allowing performance improvements,
especially ones that address real operational problems, into {{beta}} vs.
{{4.0.x?}}
{quote}
The choice isn't binary, 4.1 is more appropriate. Was just my way of stating my
concerns about this going into 4.0-beta when so much effort has been put into
the feature freeze. 4.0.x would still require a waiver IMO, but with the
QA/testing lifecycle guidelines from 4.0-beta better landed, and the stress and
anxiety of getting 4.0 out after so long, it may then be a much easier
discussion to have.
Again, can we please take this to the ML, and leave the ticket for the
technical?
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185886#comment-17185886
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
{quote}The 4.0.x line is going to be around for a while…
{quote}
{quote}that's not an argument in itself to waiver this patch into 4.0-beta, and
sets the precedence for feature creep
{quote}
That was an argument for making an incremental change now rather than waiting
for hard Java 11 support/varargs/etc., not necessarily an argument for this
being in 4.0-beta.
{quote}I would definitely vote for the incremental approach, but starting in
4.0.x
{quote}
Is there's a big difference between allowing performance improvements,
especially ones that address real operational problems, into {{beta}} vs.
{{4.0.x?}} If anything, allowing it into the former would mean we find problems
earlier. (Assuming there is a statutory prohibition on this kind of thing at
this point, I suppose that means I'm arguing for a waiver...)
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185732#comment-17185732
]
Michael Semb Wever commented on CASSANDRA-15393:
We have been strict on the 4.0 feature freeze and are now in the 4.0-beta phase.
With the agreed upon release lifecycle: I struggle with the justification for
putting this into 4.0.0, regardless of the improvements the patch brings.
If it met the criteria to go into 3.11.x then 4.0-beta is obvious. But it's not
a bug, and neither a stability/performance improvement on a 4.0 introduced
feature. Isn't this how we should be triaging all tickets from here to 4.0-rc.
{quote}The C* project has a problem with favoring rewrites in favor of
incremental improvements.
{quote}
Totally agree with you [~bdeggleston]. I would definitely vote for the
incremental approach, but starting in 4.0.x or 4.1. Especially after we have a
better QA/test process spec'd out during the 4.0-beta phase.
{quote}The 4.0.x line is going to be around for a while…
{quote}
[~maedhroz], that's not an argument in itself to waiver this patch into
4.0-beta, and sets the precedence for feature creep.
Can I suggest this needs to be raised on dev ML to get a waiver for 4.0-beta?
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185510#comment-17185510 ] Caleb Rackliffe commented on CASSANDRA-15393: - bq. The idea of combining the byte array util and the accessor has also been floating around, and I'd prefer we didn't for both the reason above, and because people are pretty used to having ByteBufferUtil handy. Having ByteArrayUtil handy would make the appearance of byte[] everywhere a little less jarring (hopefully). bq. NativeCell prevents moving most of that into AbstractCell. Given the simplicity of the parts that can be factored, I'd lean towards keeping the class hierarchy simple over minimizing the size of the array and buffer cell implementations. Agreed on both points. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185506#comment-17185506
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
bq. I'd done something sort of along these lines (albeit in the opposite
direction) by adding the `ValueAware` class, so that AbstractType et al can
operate directly on Cell objects without having to know what a cell is.
Although I seem to default to keeping type information on the serializer / type
side of things, there are benefits to each which we should discuss. And
possibly a better name for ValueAware.
Unless we have a concrete plan for {{ValueAware}}, why don't we just remove it
and use {{Cell}} directly? There might not be a real benefit to hiding the
concept of a cell from the types and serializers. My comments here (ex.
{{getLong()}}) are driving toward trying to eliminate as many places where we
actually have to explicitly pair value and accessor as possible.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185499#comment-17185499 ] Caleb Rackliffe commented on CASSANDRA-15393: - bq. I'm inclined to go the other way, and make underlying type information opaque everywhere except the serializers themselves. There are very few places where we need to see it, and making it explicit anywhere it doesn't need to be just makes it more difficult to make changes later on. I suppose my starting disposition is wondering why we wouldn't _leave_ it explicit (i.e. as it is now) if the change hurts readability/clarity for the benefit of future changes we might not actually make (or if we do make them, might not look exactly the way we imagine they will here). > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17185489#comment-17185489
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
bq. I'd prefer that we keep the type/serializers consistent wrt type. In other
words, I'd prefer some methods "unneccesarily" switch to using an accessor than
having some parts of a class use accessors and other parts use byte buffers
directly
I'm not sure. There is a clear penalty in terms of readability in places like
{{CompositeType#decompose()}}, and the main/public version of
{{AbstractType#decompose()}} already explicitly _does_ return a {{ByteBuffer}}.
This is also a case where remaining explicit reduces the sheer size of the
patch.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183659#comment-17183659 ] David Capwell commented on CASSANDRA-15393: --- bq. It might have been less of an issue, if the whole code base would have proper unit testing for all production code paths with all potential inputs or even fuzzy testing, but that's unfortunately not the case. If it helps, CASSANDRA-16064 (Caleb linked this JIRA with that one as they both created some of the same classes) provides the ability to generate random types (primitives, all collections, UDTs, reversed, etc.) and data for arbitrary schemas. Also added logic to make it a bit easier to mock out part of our code such as Schema class; the intent of CASSANDRA-16064 is to start adding such tests you talk about and lower the bar to add more. If there are sections you feel need closer testing, I can take a look if CASSANDRA-16064 offers enough for those sections, or if anything else is desired. Let me know. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17183621#comment-17183621
]
Blake Eggleston commented on CASSANDRA-15393:
-
Thanks for the excellent review [~maedhroz]. I've pushed up some commits
cleaning up a bunch of stuff (excluding docs for now) and addressing most of
the points, with the exception of the comments below.
Let me know your thoughts on them:
{quote}Looks like AbstractType#decompose() is never used without
ByteBufferAccessor? We could probably remove the type parameter and just make
the ByteBuffer binding explicit.
{quote}
and
{quote}TypeSerializer#toCQLLiteral() is only used w/ ByteBuffer, so it doesn't
look like it needs to be parameterized.
{quote}
I'd prefer that we keep the type/serializers consistent wrt type. In other
words, I'd prefer some methods "unneccesarily" switch to using an accessor than
having some parts of a class use accessors and other parts use byte buffers
directly
{quote}ModificationStatement looks like it's dealing exclusively with
ByteBuffer. Should the type parameters reflect that?
{quote}
and
{quote}Trying to propagate more typing information from
ClusteringBoundOrBoundary.Serializer upward to its users for Slices and
UnfilteredSerializer might help clarify some things
{quote}
I'm inclined to go the other way, and make underlying type information opaque
everywhere except the serializers themselves. There are very few places where
we need to see it, and making it explicit anywhere it doesn't need to be just
makes it more difficult to make changes later on.
{quote}We might benefit in terms of usability/developer ergonomics if we push
some capabilities of the accessor into Cell. (ex. methods like getLong()).
Similar thing going on with ClusteringPrefix, where we could perhaps stop
making calls like
builder.add(prefix.get[image:3144E42A-756C-4044-BF0B-371E31605DF4-68435-0002F6486CD1F067/information.png],
prefix.accessor()) and use
bufferAt[image:4A0500E8-9C94-4DC7-919E-D17260E81A8C-68435-0002F6486CE35ECB/information.png]
in CP itself. I suppose ArrayBackedBuilder might need to support something
like add(ClusteringPrefix, int) if we still want to do that lazily, after the
isDone() check.
{quote}
and
{quote}AbstractType#writeValue() could be implemented in Cell, given the latter
knows both its value and accessor already, and ColumnSpecification already
knows the column type?
{quote}
I'd done something sort of along these lines (albeit in the opposite direction)
by adding the `ValueAware` class, so that AbstractType et al can operate
directly on Cell objects without having to know what a cell is. Although I seem
to default to keeping type information on the serializer / type side of things,
there are benefits to each which we should discuss. And possibly a better name
for ValueAware.
{quote}UNSET_BYTE_ARRAY, getChar(), putBoolean(), putChar(), writeWithLength()
are unused in ByteArrayUtil. What if we just fold these methods into
ByteArrayAccessor.
{quote}
These are all copied from java.io.Bits, so I assume the logic is correct and
well tested. I'd like to leave them unused if you don't mind. ValueAccessor use
is restricted to reading for the most part, but as it gets expanded to cover
writing things, I think having known good implementations ready to go would be
more beneficial than starting with a minimal implementation.
The idea of combining the byte array util and the accessor has also been
floating around, and I'd prefer we didn't for both the reason above, and
because people are pretty used to having ByteBufferUtil handy. Having
ByteArrayUtil handy would make the appearance of byte[] everywhere a little
less jarring (hopefully).
{quote}We might be able to factor our some common elements of ArrayCell and
BufferCell.
{quote}
NativeCell prevents moving most of that into AbstractCell. Given the simplicity
of the parts that can be factored, I'd lean towards keeping the class hierarchy
simple over minimizing the size of the array and buffer cell implementations.
> 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
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17176410#comment-17176410 ] Caleb Rackliffe commented on CASSANDRA-15393: - bq. I disagree with the idea that we should delay improving compaction allocations because we could implement a better solution at some point in the future. Agreed. The 4.0.x line is going to be around for a while, so I lean toward trying to make the best of the tools at our disposal in Java 8. I think we'd have to make the argument that we'd be doing real damage to the codebase by continuing. bq. I don't personally think there is an issue with being able to invoke in a contrived example where we deliberately elide the type information, but we should be sure the code doesn't do that today. Agreed. This seems like something we should be able to catch in review pretty easily, although perhaps in the past we've not always been very strict about making sure we either eliminate compiler warnings or clearly document why we can't. bq. The most complex parts are probably the collection serializers and other places where we're now having to do offset bookkeeping. I know I'm going to be spending more time on both of these things in my second pass at review (there are already enough failing tests and other things that need cleanup first), and we almost certainly don't have sufficient test coverage, but serializers and offset manipulation logic aren't _that_ tricky? bq. It might have been less of an issue, if the whole code base would have proper unit testing for all production code paths with all potential inputs or even fuzzy testing, but that's unfortunately not the case. We do have the entire CASSANDRA-15536 epic (including fuzz testing w/ Harry) in progress, which will subject anything we do here to quite a bit of additional scrutiny, even after we get the testing within this issue itself to the bar. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175656#comment-17175656 ] Blake Eggleston commented on CASSANDRA-15393: - bq. I'm sorry, if my objections sound harsh. But my point is that it's better to fix the whole heap-pressure-nightmare with (de)serialization (reads/writes/re-serializations/compaction/etc) in the next major release. It’s fine, we should be talking about these things. I disagree with the idea that we should delay improving compaction allocations because we could implement a better solution at some point in the future. It's a textbook example of “letting perfect be the enemy of good enough”. The C* project has a problem with favoring rewrites in favor of incremental improvements. Compaction heap pressure is a real operational problem that causes a lot of headaches, and there is real value in mitigating it as part of 4.0, even if it can be further improved in the future. There's certainly risk here, but I think it's being a bit overstated. The changes here are wide, but not particularly deep. The most complex parts are probably the collection serializers and other places where we're now having to do offset bookkeeping. These should be carefully reviewed, but they're hardly unverifiable. bq. Unrelatedly, [Blake Eggleston](https://issues.apache.org/jira/secure/ViewProfile.jspa?name=bdeggleston) , is there a reason you didn't go the whole hog and just get rid of the ByteBuffer versions of everything? 1) That would have been a much larger change, and I wanted to limit scope. IIRC, replacing partition keys would have been a lot of work for a comparatively small gc win. 2) bytebuffers are still useful in some places. Specifically in places where we're using allocators 3) I've been working on a flyweight reader in my free time that reduces another 95% of garbage and uses bytebuffers. This will be ready for 4.next, but using it should be optional. 4) There's value in decoupling data format from data logic. For instance, this would allow us to compare native and bytebuffer values without requiring the allocation of an intermediate buffer. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175390#comment-17175390
]
Benedict Elliott Smith commented on CASSANDRA-15393:
{quote}This one compiles without errors just by omitting the generic type
information: This one compiles without errors just by omitting the generic type
information
{quote}
This seems like a problem that can be resolved - I agree that some of the type
information isn't properly propagated in some places, but I suspect that might
be because the patch was awaiting feedback on its general structure.
{quote}Another source of errors is that positions & offsets are manually
handled now.
{quote}
I am also not a _huge_ fan of this, but we have compromises to make. I don't
think this is particularly error-prone, however, since we already do this a
great deal - it's pretty integral to how our serialization/deserialization
works, so this is not a new category of problem. That's not to say there isn't
room for suggestions that might lead to improvement here, I don't know, but it
doesn't seem at all disqualifying to me.
Unrelatedly, [~bdeggleston], is there a reason you didn't go the whole hog and
just get rid of the {{ByteBuffer}} versions of everything?
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175291#comment-17175291
]
Robert Stupp commented on CASSANDRA-15393:
--
This one compiles without errors just by omitting the generic type information:
{code}
ValueAccessor acc = ByteArrayAccessor.instance;
CounterContext.headerLength(ByteBuffer.allocate(10), acc);
{code}
^ This is a verbose example e.g. of the method signatures
{{o.a.c.db.rows.RangeTombstoneMarker#openBound}} + {{closeBound}} /
{{o.a.c.db.Clusterable#clustering}} - and actual production code like the above
(e.g. in {{o.a.c.db.MutableDeletionInfo.Builder#add}}). It seems that the
existing production code base including the unchanged parts with all call sites
need a full re-review to validate that this change is safe.
Another source of errors is that positions & offsets are manually handled now.
For example
[here|https://github.com/apache/cassandra/pull/712/files#diff-a9c2759ec4c4663dd4a24d5e0f89358fR225]
or
[here|https://github.com/apache/cassandra/pull/712/files#diff-1e045cbe2fa513c1d2e7ac63c1b764ccR58].
Just think about merging a change from the 3.x line into trunk but not adding
the necessary position/offsets arithmetics.
As I said, there's definitive value in reducing heap pressure in critical hot
paths. But my concern that the approach is error prone due to the issues
mentioned here still stands.
It might have been less of an issue, if the whole code base would have proper
unit testing for all production code paths with all potential inputs or even
fuzzy testing, but that's unfortunately not the case.
The risks are:
* Mis-computing offsets+positions can lead to potentially unnoticed data
corruption, overflows, etc that initially go unnoticed
* Spurious new runtime errors in rare situations (barely tested ones) leading
to ClassCastExceptions
I'm sorry, if my objections sound harsh. But my point is that it's better to
fix the whole heap-pressure-nightmare with (de)serialization
(reads/writes/re-serializations/compaction/etc) in the next major release.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175221#comment-17175221 ] Caleb Rackliffe commented on CASSANDRA-15393: - In terms of the higher level question of whether a change like this belongs in 4.0, I'm generally of the opinion that it does, given that it benefits both compaction and local reads. There are things to clean up, but once that happens, it really should be pretty hard to make the kind of errors mentioned [above|https://issues.apache.org/jira/browse/CASSANDRA-15393?focusedCommentId=17173009=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17173009]. If I were arguing against including this in 4.0, I would probably point to the sheer size of the patch and the amount of typing information it adds to existing classes, although the new type information on classes and method signatures means the number of lines in the diff is kind of misleading as a measure of how risky the change is. Unless we think the patch will both hamper future efforts to avoid materializing objects on heap in the first place during compaction, I don't see a strong reason not to move forward. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175217#comment-17175217
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
While we continue to hash out the more strategic discussion, here are the notes
from my first pass at review:
(Note: These comments are based on things as they stand in [my
rebase|https://github.com/apache/cassandra/pull/712].)
*Naming Ideas*
- {{ClusteringPrefix#getBuffer(int)}} -> {{bufferAt(int)}}
- {{ClusteringPrefix#getString(int)}} -> {{stringAt(int)}}
- {{ValueAccessor#toSafeBuffer()}} -> {{toMutableBuffer()}}
*Typing*
- It looks safe to throw {{@SafeVarargs}} on {{CompositeType#build()}}.
- {{TypeSerializer#validate()}} and {{AbstractType#writeValue()}} hide the
class-level type parameter.
- Looks like {{AbstractType#decompose()}} is never used without
{{ByteBufferAccessor}}? We could probably remove the type parameter and just
make the {{ByteBuffer}} binding explicit.
- {{TypeSerializer#toCQLLiteral()}} is only used w/ {{ByteBuffer}}, so it
doesn't look like it needs to be parameterized.
- {{ModificationStatement}} looks like it's dealing exclusively with
{{ByteBuffer}}. Should the type parameters reflect that?
- Trying to propagate more typing information from
{{ClusteringBoundOrBoundary.Serializer}} upward to its users for {{Slices}} and
{{UnfilteredSerializer}} might help clarify some things. (i.e. Literally return
{{ClusteringBoundOrBoundary}} from {{deserializeValues()}}. We could
create an array-based version of TOP and BOTTOM.)
- The {{MultiCBuilder}} sub-classes seem to be used only w/ {{ByteBuffer}}.
Make sure it's types are explicit about that?
- Assuming we don't find some creative ways to reduce the number of places
where we need type
parameters, the following classes also need to be rounded out to avoid
compile-time warnings:
-- {{BufferClusteringBoundOrBoundary}}, {{ClusteringPrefix}},
{{ClusteringBound}}, {{ClusteringBoundOrBoundary}}
-- {{Row}} (on Cell return types), {{Cell}}, {{Array}} / {{BufferCell}},
{{ColumnMetadata}}, {{BTreeRow}}, {{ComplexColumnData}}, {{Slice}}
-- {{CounterContext}} (ex. {{total()}})
-- {{RegularColumnIndex}}, {{IndexEntry}}, {{CollectionKeyIndex}},
{{CollectionKeyIndexBase}}, {{CassandraIndex}}
-- {{AbstractAllocator}}
-- {{CBuilder}} (and {{ArrayBackedBuilder}}...also {{ArrayBackedBuilder}} only
builds {{BufferClustering}}? If so, that should flow up in
the type information to {{RegularColumnIndex}}, et al.)
*Safety Concerns*
- {{AbstractArrayClusteringPrefix}} references it's own sub-class in a static
context...we might need to move the empty size to {{ArrayClustering}}?
*Code Organization*
- Would be nice to avoid the switch in {{ClusteringBoundary}} and
{{ClusteringBound}}. If we know this only works with the {{ByteBuffer}}
version, can we just cleanly separate the static factories from the interfaces?
- We might be able to factor our some common elements of {{ArrayCell}} and
{{BufferCell}}.
- We might benefit in terms of usability/developer ergonomics if we push some
capabilities of the accessor into {{Cell}}. (ex. methods like {{getLong()}}).
Similar thing going on with {{ClusteringPrefix}}, where we could perhaps stop
making calls like {{builder.add(prefix.get(i), prefix.accessor())}} and use
{{bufferAt(i)}} in CP itself. I suppose {{ArrayBackedBuilder}} might need to
support something like {{add(ClusteringPrefix, int)}} if we still want to do
that lazily, after the {{isDone()}} check.
- {{AbstractType#writeValue()}} could be implemented in {{Cell}}, given the
latter knows both its value and accessor already, and {{ColumnSpecification}}
already knows the column type?
*TODOs and FIXMEs*
- Would {{NativeCell}} not having a specialized accessor be any worse than the
existing codebase? (If not, I'd be okay with deferring it to another Jira...?)
- It looks like there are a few places where you wanted to avoid
{{ClusteringPrefix#getBufferArray()}}. (ex. {{RangeTombstoneMarker.Merger}}).
I'm not sure how many of those usages actually suffer from the wrapping
overhead though.
- {{validateCollectionMember}}, the logger, and {{componentsCount}} are unused
in {{AbstractType}}.
- {{deserializeValuesWithoutSize()}} unused in {{ClusteringPrefix}} (and
{{version}} is unused in the other overload?)
- Most of the static factories in {{ArrayClusteringBoundOrBoundary}} and
{{ArrayClusteringBound}} are unused.
- {{UNSET_BYTE_ARRAY}}, {{getChar()}}, {{putBoolean()}}, {{putChar()}},
{{writeWithLength()}} are unused in {{ByteArrayUtil}}. What if we just fold
these methods into {{ByteArrayAccessor}}.
- FIXME in {{AbstractClusteringPrefix}} ...modify {{Digest}} to take a
value/accessor pair?
- Some items commented out near the top of {{BufferClusteringBound}}.
- TypeSerializer#serialize() and serializeBuffer() don't both need to exist?
-
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17175119#comment-17175119 ] Blake Eggleston commented on CASSANDRA-15393: - bq. The split of ByteBuffer into a pair of accessor + container is error prone - e.g. accidentally passing a byte[] with the ByteBufferAccessor or vice versa leads to "interesting" results. Could you elaborate on how this would happen [~snazy]? I've structured the classes to avoid this problem specifically. The value accessor's type parameter needs to be the same as the value's type, mixing them up should result in a compile error. Is there somewhere this doesn't work as intended? > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17173037#comment-17173037
]
Benedict Elliott Smith commented on CASSANDRA-15393:
{quote}But introducing an API that's error prone is dangerous. But introducing
an API that's error prone is dangerous.
{quote}
This particular issue can be somewhat resolved by using generic parameters for
methods that accept the object and accessor, including the constructor of any
object holding them, e.g.
{code:java}
public interface Accessor {}
public class SomethingWithData
{
final Object data;
final Accessor accessor;
public SomethingWithData(D data, Accessor accessor) {
this.data = data; this.accessor = accessor; }
} {code}
This avoids polluting the codebase with type parameters, but ensures the first
supplier of the Object+Accessor is safe, and later supplies are safe if they
only propagate pairs produced in this way.
We could also probably generify the objects themselves, I don't think it would
be too painful - but this at least is an easy and less invasive improvement.
{quote}The underlying issue IMO is not {{ByteBuffer}} itself but the vast
amount of those intermediate and often tiny BB instances.
{quote}
I don't have a strong opinion on the overall structural impact of this change;
I haven't thought through the various options. However I don't think we
produce all that many intermediate {{ByteBuffer}} specifically, and though we
do produce a lot of intermediate objects in many cases, there are workloads
where this isn't the issue, and object creation is still a problem.
When it comes to compaction, my preferred approach would be to avoid
materialising on heap at all - but the change here helps both compaction and
reads/writes, so it doesn't have to be either/or, and we should definitely be
trying to reduce our heap footprint in general.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17173009#comment-17173009
]
Robert Stupp commented on CASSANDRA-15393:
--
Thanks for tackling this! Heap pressure induced by {{ByteBuffer}} is definitely
a pita. I'm not sure though whether this patch is suitable to be included in
4.0 this late (beta stage) as it looks quite intrusive and is pretty big.
Haven't thoroughly reviewed it, but here are a few notes:
* The new *Accessor classes have constants for boolean (true/false)
representations, which are modifiable. IIRC there's been an issue in the past
when that (or a similar) constant was accidentally changed.
* The split of {{ByteBuffer}} into a pair of {{accessor + container}} is error
prone - e.g. accidentally passing a {{byte[]}} with the {{ByteBufferAccessor}}
or vice versa leads to "interesting" results.
The underlying issue IMO is not {{ByteBuffer}} itself but the vast amount of
those intermediate and often tiny BB instances. But introducing an API that's
error prone is dangerous.
It feels both more efficient and much safer to have an API & implementation
that actually _prevents_ (the vast majority of) these new objects. I.e. instead
of creating a new {{ByteBuffer}} (or "just" a new {{byte[]}} as in the PR) to
write some value (e.g. an {{int}}) to a target buffer, write that value
_directly_ into the target buffer. _How_ that's implemented is a different
topic, but I'd prefer to consider all the new developments in Java and make it
safe & transparent to use.
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17172676#comment-17172676 ] Caleb Rackliffe commented on CASSANDRA-15393: - There is a fair number of unit test [failures|https://app.circleci.com/pipelines/github/maedhroz/cassandra/94/workflows/9a0adbdc-56ae-48fd-91a7-1104afcba560/jobs/463], although they seem to mostly predate the rebase. > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17172612#comment-17172612
]
Caleb Rackliffe commented on CASSANDRA-15393:
-
[~bdeggleston] I made a pass at a rebase, given the changes made in trunk
around CASSANDRA-15400, CASSANDRA-15461, and CASSANDRA-14365 since we last
touched this. You can find it
[here|https://github.com/apache/cassandra/pull/712] (and a link to a CircleCI
run is in the comments).
Notes:
- {{CompactionAllocationTest}} already exists on trunk, so I more or less took
its modifications during conflict resolution.
- I've
[preserved|https://github.com/apache/cassandra/pull/712/files#diff-e65bcd2d398327679a75a9da82473635R45]
the FIXME in the digest update.
- There are numerous places where I've added proper type/generics information
to method signatures. It probably inflated the diff a bit.
- I think I've implemented {{ClusteringPrefix#minimize()}} reasonably across
the native, array-backed, and bb-backed classes, but that's an area for us to
review.
With all of that cleaned up, I'll start on actually reviewing the patch :)
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17171803#comment-17171803 ] Caleb Rackliffe commented on CASSANDRA-15393: - [~marcuse] [~samt] I'm going to make a pass at review here if you two don't mind... > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16986441#comment-16986441
]
Blake Eggleston commented on CASSANDRA-15393:
-
[~benedict], I’d like your opinion on this. Instead of adding a generic
{{Value}} wrapper, I tried using the same opaque value / value accessor pattern
I’d used on CASSANDRA-15390 to generalize serialization logic. Code is
[here|https://github.com/bdeggleston/cassandra/tree/15393-accessor].
Decoupling the serialization logic from the memory abstraction like this makes
it possible to realize all the allocation improvements of using byte arrays for
cell values without locking us into byte arrays or adding overhead of a wrapper
object (which incurs a ~6% allocation penalty with these tests). We also avoid
worrying about megamorphic call sites.
As a bonus, this pattern means we can convert clustering prefix to use byte
arrays as well without a lot of extra work, which saves an additional 5% of
allocations, on top of the existing ~18% from cells.
So that’s the good news, the bad news is that it touches a lot of serialization
code, and is just a larger patch in general than simply converting cells to use
byte arrays (although I’m not sure how well that would have worked with the
slab allocator in practice). I still need to chase down a handful of failing
tests and there are some rough edges around how value/accessors are handled via
generic wildcards that need to be worked out.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16967116#comment-16967116
]
Blake Eggleston commented on CASSANDRA-15393:
-
Here's a rough initial pass at converting cells from buffers to arrays:
[https://github.com/bdeggleston/cassandra/tree/15387-arrays]
There are {{FIXME}}s everywhere and I still need to clean up some ideas that
didn't pan out and take another look at cell cloning, but +902/-402 is smaller
than I was expecting.
I've also started playing around with converting from buffer to a {{Value}}
type that can be backed by ByteBuffer, byte[], or native memory. It's nicer to
look at, but is also a much deeper rabbit hole. There is also code in a lot of
places that relies on ByteBuffers having mutable position that would need to
reworked and verified.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964309#comment-16964309
]
Blake Eggleston commented on CASSANDRA-15393:
-
{quote}Yes, remove it entirely from the API (and perhaps more widely from the
codebase, although this could be done incrementally)
{quote}
That would probably be the most straightforward approach for trunk. There may
be some sharp edges outside of the local read path but it's worth looking into.
I've been working on a garbage free reader that relies on having some wrapper
over the raw bytes, but that would be a post-4.0 project and we could go back
to byte buffers if/when it materializes.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964284#comment-16964284 ] Benedict Elliott Smith commented on CASSANDRA-15393: Thanks - I now see that it's right there in the spreadsheet if I had looked at the calculation > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964279#comment-16964279 ] Blake Eggleston commented on CASSANDRA-15393: - The row for this patch measures the delta between 15393 and 15391 against the baseline established in 15388 > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964278#comment-16964278 ] Benedict Elliott Smith commented on CASSANDRA-15393: Yes, remove it entirely from the API (and perhaps more widely from the codebase, although this could be done incrementally) > The linked spreadsheet has 3 tabs Thanks, that's what I was missing. How is this being measured, given that this patch builds upon other patches that reduce allocations? > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964267#comment-16964267
]
Blake Eggleston commented on CASSANDRA-15393:
-
> Would the local-read benefits be better served by removing ByteBuffer
> entirely?
Could you clarify that? Do you mean remove ByteBuffer from the cell interface
entirely and using byte arrays for everything?
The linked spreadsheet has 3 tabs, the "Tiny" tab you're probably looking at is
just measuring the {{(k int primary key, v int)}} data model. The medium and
wide tabs are workloads represent more common workloads.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964251#comment-16964251
]
Benedict Elliott Smith commented on CASSANDRA-15393:
Would the local-read benefits (as well as many others) be better served by
removing {{ByteBuffer}} entirely? It's more involved, but again has no
negative repercussions we need to mitigate, and is a legacy we need to shed
eventually.
Could you also clarify the improvement numbers? The
[spreadsheet|https://docs.google.com/spreadsheets/d/1_m2qoJYkyM3-W95Ex8yxqPtE48lJcKgeaUlsvHrInmA/edit#gid=0]
you linked suggests this only improves ~1-2%?
We could also improve the local-read path in a similar garbage-free manner, but
I would prefer to refactor more deeply to support that safely.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964231#comment-16964231 ] Blake Eggleston commented on CASSANDRA-15393: - No, but the local read path also benefits from most of these changes. > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964228#comment-16964228 ] Benedict Elliott Smith commented on CASSANDRA-15393: This shouldn't ever be used for compaction I don't think? > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964227#comment-16964227 ] Blake Eggleston commented on CASSANDRA-15393: - The only exception I'm aware of is the reverse sstable iterator. > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964215#comment-16964215
]
Benedict Elliott Smith commented on CASSANDRA-15393:
So first we would have to ensure we materialise to the heap anything we hold
onto with unknown lifetime (e.g. {{ColumnIndex.firstClustering}}, much like
with flushing). I don't think there are many such places.
We could then use the fact that an {{Unfiltered}} is immediately discarded
after being written, to keep in memory at most 2 {{Unfiltered}} - the one we're
maybe using, and the one that might now have been peeked at for use in the
{{MergeIterator}}.
I don't think anything more should be needed, but we'd have to look carefully.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16964202#comment-16964202 ] Blake Eggleston commented on CASSANDRA-15393: - I had not, this group of changes was developed on 3.0 then ported up to trunk. That would neatly fix the concerns with this ticket and CASSANDRA-15391 though. Something that's not immediately clear to me (and I'm not really familiar with off heap memtables at all), is how we'd prevent the amount of memory used off heap from growing too large. Reclaiming after the parent partition has been closed would be too long to wait in the case of huge partitions, and the lifecycle of Unfiltered instances rely on the garbage collector cleaning things up. Any thoughts there? > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[
https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16963950#comment-16963950
]
Benedict Elliott Smith commented on CASSANDRA-15393:
Have we considered, for compaction, materialising {{NativeCell}} from disk
instead? These have only 16 or 24 bytes of heap overhead, depending on the JVM
settings, and avoid materialising any of the payload. We would need to
parameterise our {{Serializer}}, and for compaction associate it with a
{{NativeAllocator}} for producing {{NativeCell}}, and reclaim this after each
partition (or each row of a partition). Even the {{NativeCell}} themselves
could be reused in this scheme, giving zero `Cell` related garbage.
> 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
>
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15393) Add byte array backed cells
[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16963872#comment-16963872 ] Benedict Elliott Smith commented on CASSANDRA-15393: We have to be careful here, because this could have serious negative consequences for users employing off heap memtables, since all Cell operations will become megamorphic. I don't think we can introduce this change without fairly extensive performance testing. > 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 > > > 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-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
