You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/03/29 14:06:30 UTC
[GitHub] [skywalking-java] Oliverwqcwrw opened a new pull request #132: optimize charset
Oliverwqcwrw opened a new pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132
In `org.apache.skywalking.apm.toolkit.opentracing.ByteBufferContext`
Optimized acquisition charset
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132#discussion_r838045628
##########
File path: apm-application-toolkit/apm-toolkit-opentracing/src/main/java/org/apache/skywalking/apm/toolkit/opentracing/ByteBufferContext.java
##########
@@ -21,11 +21,12 @@
import io.opentracing.SpanContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
public class ByteBufferContext implements SpanContext {
- static final Charset CHARSET = Charset.forName("UTF-8");
+ static final Charset CHARSET = StandardCharsets.UTF_8;
Review comment:
This is a static field, only loaded once in class loader.
So, I doubt the performance impact.
Any benchmark you could share? It is hard to be specific, I think, maybe only nano second difference?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng closed pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
wu-sheng closed pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132#discussion_r837614921
##########
File path: apm-application-toolkit/apm-toolkit-opentracing/src/main/java/org/apache/skywalking/apm/toolkit/opentracing/ByteBufferContext.java
##########
@@ -21,11 +21,12 @@
import io.opentracing.SpanContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
public class ByteBufferContext implements SpanContext {
- static final Charset CHARSET = Charset.forName("UTF-8");
+ static final Charset CHARSET = StandardCharsets.UTF_8;
Review comment:
I think SkyWalking community usually doesn't argue about code style.
I have seen dozens of code styles, due to different countries, education env, first language background, etc.
I and PMC don't want to tell contributors how to write codes, unless they are wrong, hard to read/maintain, performance impact.
@Oliverwqcwrw Please forgive I can't support this kind of changes.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] Superskyyy commented on a change in pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
Superskyyy commented on a change in pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132#discussion_r837610689
##########
File path: apm-application-toolkit/apm-toolkit-opentracing/src/main/java/org/apache/skywalking/apm/toolkit/opentracing/ByteBufferContext.java
##########
@@ -21,11 +21,12 @@
import io.opentracing.SpanContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
public class ByteBufferContext implements SpanContext {
- static final Charset CHARSET = Charset.forName("UTF-8");
+ static final Charset CHARSET = StandardCharsets.UTF_8;
Review comment:
> UTF-8 is a built-in Charset. What is this `optimization` actually about?
I think it's more of a chore/styling-wise change. The performance is exactly the same.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132#discussion_r837594815
##########
File path: apm-application-toolkit/apm-toolkit-opentracing/src/main/java/org/apache/skywalking/apm/toolkit/opentracing/ByteBufferContext.java
##########
@@ -21,11 +21,12 @@
import io.opentracing.SpanContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
public class ByteBufferContext implements SpanContext {
- static final Charset CHARSET = Charset.forName("UTF-8");
+ static final Charset CHARSET = StandardCharsets.UTF_8;
Review comment:
UTF-8 is a built-in Charset. What is this `optimization` actually about?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] Oliverwqcwrw commented on a change in pull request #132: optimize charset
Posted by GitBox <gi...@apache.org>.
Oliverwqcwrw commented on a change in pull request #132:
URL: https://github.com/apache/skywalking-java/pull/132#discussion_r838024880
##########
File path: apm-application-toolkit/apm-toolkit-opentracing/src/main/java/org/apache/skywalking/apm/toolkit/opentracing/ByteBufferContext.java
##########
@@ -21,11 +21,12 @@
import io.opentracing.SpanContext;
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
public class ByteBufferContext implements SpanContext {
- static final Charset CHARSET = Charset.forName("UTF-8");
+ static final Charset CHARSET = StandardCharsets.UTF_8;
Review comment:
First of all, I am sorry that I did not specify the reason for optimization,The reason I optimized this is because I tested it both ways
> Charset.forName("UTF-8") init spending nanoTime: 69553
StandardCharsets.UTF_8 init spending nanoTime 12120
So my initial idea was not to change the code style, but for performance reasons
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org