You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/12/16 01:20:50 UTC
[GitHub] [ignite] aries-3 opened a new pull request #8577: Ignite 13856 DirectByteBufferStreamImplV2.writeString method is changed. String.getBytes() is called only once.
aries-3 opened a new pull request #8577:
URL: https://github.com/apache/ignite/pull/8577
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] akalash commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
akalash commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r546651327
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -584,7 +587,21 @@ public DirectByteBufferStreamImplV2(MessageFactory msgFactory) {
/** {@inheritDoc} */
@Override public void writeString(String val) {
- writeByteArray(val != null ? val.getBytes() : null);
+ if (val != null) {
+ if (buf.capacity() < val.length()) {
Review comment:
I don't think that this condition is right. for example if 'buf.capacity() > val.length()' and 'buf.remaining() < val.length()' that it still requires to cache the bytes but in your case this won't happen. So maybe it is better to remove that condition at all, I don't think that writing to the local variable is too expensive so you can do it every time even it isn't required for certain case. Or you should change you condition to the proper one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alamar commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
alamar commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r548873780
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -301,6 +301,9 @@
/** */
protected boolean lastFinished;
+ /** byte-array representation of string */
+ private byte[] byteArr;
Review comment:
@akalash says volatile is not needed. Can you please just rename this field as you see fit?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alamar commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
alamar commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r548872849
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -584,7 +587,17 @@ public DirectByteBufferStreamImplV2(MessageFactory msgFactory) {
/** {@inheritDoc} */
@Override public void writeString(String val) {
- writeByteArray(val != null ? val.getBytes() : null);
+ if (val != null) {
+ if (byteArr == null)
+ byteArr = val.getBytes();
+
+ writeByteArray(byteArr);
+
+ if (lastFinished)
+ byteArr = null;
+ }
+ else
+ writeByteArray(null);
Review comment:
Actually, we need to reset it to null if lastFinished. Need to figure out the logic, maybe it's too cumbersome and confusing to implement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alamar commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
alamar commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r548871960
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -301,6 +301,9 @@
/** */
protected boolean lastFinished;
+ /** byte-array representation of string */
+ private byte[] byteArr;
Review comment:
Can we perhaps make it a `private volatile` and name smth like `curStrBackingArr`? I'm not confident that it will be called from the same thread.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -584,7 +587,17 @@ public DirectByteBufferStreamImplV2(MessageFactory msgFactory) {
/** {@inheritDoc} */
@Override public void writeString(String val) {
- writeByteArray(val != null ? val.getBytes() : null);
+ if (val != null) {
+ if (byteArr == null)
+ byteArr = val.getBytes();
+
+ writeByteArray(byteArr);
+
+ if (lastFinished)
+ byteArr = null;
+ }
+ else
+ writeByteArray(null);
Review comment:
can we perhaps declare byteArr a local variable, and only assign it to curStrBackingArr if not lastFinished? This way we will not touch this variable at all when string fits one frame.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alamar commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
alamar commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r548872973
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -584,7 +587,17 @@ public DirectByteBufferStreamImplV2(MessageFactory msgFactory) {
/** {@inheritDoc} */
@Override public void writeString(String val) {
- writeByteArray(val != null ? val.getBytes() : null);
+ if (val != null) {
+ if (byteArr == null)
+ byteArr = val.getBytes();
+
+ writeByteArray(byteArr);
+
+ if (lastFinished)
+ byteArr = null;
+ }
+ else
+ writeByteArray(null);
Review comment:
Disregard this, we are going to read curStrBackingArr value either way. I think we may skip this change entirely, just rename var / mark volatile.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] asfgit closed pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #8577:
URL: https://github.com/apache/ignite/pull/8577
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] akalash commented on a change in pull request #8577: IGNITE-13856 make linear performance for DirectByteBufferStreamImplV2.writeString
Posted by GitBox <gi...@apache.org>.
akalash commented on a change in pull request #8577:
URL: https://github.com/apache/ignite/pull/8577#discussion_r545905042
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -301,6 +302,9 @@
/** */
protected boolean lastFinished;
+ /** map for cashing byte-array representations of strings */
+ private Map<String, byte[]> stringsMap;
Review comment:
Why are you using a map here? A simple byte array is enough, isn't it?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/direct/stream/v2/DirectByteBufferStreamImplV2.java
##########
@@ -584,7 +588,29 @@ public DirectByteBufferStreamImplV2(MessageFactory msgFactory) {
/** {@inheritDoc} */
@Override public void writeString(String val) {
- writeByteArray(val != null ? val.getBytes() : null);
+ if (val != null) {
+ if (buf.capacity() < val.length()) {
+ if (stringsMap == null)
+ stringsMap = new HashMap<>();
+
+ byte[] bytes = stringsMap.computeIfAbsent(val, s -> s.getBytes());
+
+ try {
+ writeByteArray(bytes);
+ }
+ catch (Exception e) {
Review comment:
I believe if an exception appears here, this whole object will be invalid, for example, arrOff would be incorrect, so perhaps it's not necessary to handle this exception here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org