[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14631095#comment-14631095
]
Benedict commented on CASSANDRA-9797:
-
Nah. If CI is happy, patch LGTM
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x, 2.2.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14631017#comment-14631017
]
Sylvain Lebresne commented on CASSANDRA-9797:
-
bq. I mean reintroduce the write(byte[]...) implementation as we had it then,
since it likely introduces fewer risks to restore behaviour as it was than to
rewrite it.
That would make sense, though looking at that more closely, there has been
enough change to {{SequentialWriter}} than just copy-pasting the old version
doesn't at all (that old version uses {{bufferCursor()}} that doesn't exist
anymore, it sets {{current}} even though that's not a method not a field, and
sets {{validBufferBytes}} that also doesn't exist anymore). So we would need to
revert a little bit more than just that one method, or modify it to fit the new
stuffs, but both of which looks a lot more risky that the very simple version
attached. That said, I'm also not the most familiar with the different
evolution of {{SequentialWriter}} so if someone feels more confident with one
of those two previous option, happy to let you have a shot at it.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14629922#comment-14629922
]
Benedict commented on CASSANDRA-9797:
-
When I say revert, I mean reintroduce the write(byte[]...) implementation as we
had it then, since it likely introduces fewer risks to restore behaviour as it
was than to rewrite it.
I don't mind tremendously though. I haven't looked closely at the patch, but it
looks reasonable.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14629917#comment-14629917
]
Sylvain Lebresne commented on CASSANDRA-9797:
-
bq. I think perhaps the correct thing to do is partially revert CASSANDRA-8709
I'm not familiar with that patch in the first place so no particular opinion on
that a priori. That said, from a quick glance it looks like the attached patch
does fix {{writeBytes}} and {{writeUTF}} and the patch is simple enough that
it's maybe simpler than reverting parts of CASSANDRA-8709 (until CASSANDRA-9500
lands that is)? Unless you have worries about the patch of course.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14629895#comment-14629895
]
Benedict commented on CASSANDRA-9797:
-
{quote}
so, let's shit it.
EDIT: (accidental yes/no swap)
{quote}
Great editing ;)
More seriously, this looks to affect 2.2, and any writeBytes or writeUTF call,
which could be very costly. I think perhaps the correct thing to do is
partially revert CASSANDRA-8709, which is what introduced this change.
CASSANDRA-9500 is the best solution, but not sure when we'll get to that. I had
been hoping it would be in by 3.0, but it's looking unlikely.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14626215#comment-14626215
]
Robert Stupp commented on CASSANDRA-9797:
-
Oh - yes. Two timeouts on cassci and some {{java.lang.OutOfMemoryError: GC
overhead limit exceeded}} in {{CqlTableTest}}. I just looked at the number of
failures (31 vs. 12).
I don't think these failures are related to this patch. So I revert my revert
of +1 - so, let's shit it.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14626204#comment-14626204
]
Sylvain Lebresne commented on CASSANDRA-9797:
-
bq. cassci complains
Seems to be mostly what is already failing on trunk. Do you see anything
specific that sounds related to this patch?
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14626196#comment-14626196
]
Robert Stupp commented on CASSANDRA-9797:
-
cstar is happy with your patch :)
http://cstar.datastax.com/tests/id/06aa908e-2a0b-11e5-ac85-42010af0688f
GC stats look better
>From my POV: +1
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14626070#comment-14626070
]
Sylvain Lebresne commented on CASSANDRA-9797:
-
You have my benchmark code, it's {{cassandra-stress write n=200 -rate
threads=50}}. I haven't attempted to micro-benchmark that at all.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9797) Don't wrap byte arrays in SequentialWriter
[
https://issues.apache.org/jira/browse/CASSANDRA-9797?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14626067#comment-14626067
]
Robert Stupp commented on CASSANDRA-9797:
-
[~slebresne] can you provide your benchmark code (even if it's ugly)? "Hacking"
a JMH is not a big deal - JMH (in the newer versions) also provides nice GC
stats. Can help with that.
> Don't wrap byte arrays in SequentialWriter
> --
>
> Key: CASSANDRA-9797
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9797
> Project: Cassandra
> Issue Type: Improvement
>Reporter: Sylvain Lebresne
>Assignee: Sylvain Lebresne
>Priority: Minor
> Labels: performance
> Fix For: 3.x
>
> Attachments: 9797.txt
>
>
> While profiling a simple stress write run ({{cassandra-stress write n=200
> -rate threads=50}} to be precise) with Mission Control, I noticed that a non
> trivial amount of heap pressure was due to the {{ByteBuffer.wrap()}} call in
> {{SequentialWriter.write(byte[])}}. Basically, when writing a byte array, we
> wrap it in a ByteBuffer to reuse the {{SequentialWriter.write(ByteBuffer)}}
> method. One could have hoped this wrapping would be stack allocated, but if
> Mission Control isn't lying (and I was told it's fairly honest on that
> front), it's not. And we do use that {{write(byte[])}} method quite a bit,
> especially with the new vint encodings since they use a {{byte[]}} thread
> local buffer and call that method.
> Anyway, it sounds very simple to me to have a more direct {{write(byte[])}}
> method, so attaching a patch to do that. A very quick local benchmark seems
> to show a little bit less allocation and a slight edge for the branch with
> this patch (on top of CASSANDRA-9705 I must add), but that local bench was
> far from scientific so happy if someone that knows how to use our perf
> service want to give that patch a shot.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
