You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2024/06/07 09:00:12 UTC
[PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
nfsantos opened a new pull request, #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511
Microbenchmarks:
Adapted from JsopStreamTest:
```
public static void main(String... args) {
for (int k = 0; k < 5; k++) {
String s = "Hello \"World\" Hello \"World\" Hello \"World\" Hello \"World\" Hello \"World\" Hello \"World\" ";
StopWatch timer = new StopWatch();
JsopBuilder w = new JsopBuilder() ;
for (int i = 0; i < 50000000; i++) {
w.value(s);
if (i % 100 == 0) {
w.resetWriter();
}
}
System.out.println(w.getClass() + ": " + timer.seconds());
}
}
```
Base line:
```
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 9.27 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 9.19 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 9.21 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 9.23 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 9.20 seconds
```
Optimized StringBuilder. value(String)
```
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 8.09 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 8.04 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 8.12 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 8.22 seconds
class org.apache.jackrabbit.oak.commons.json.JsopBuilder: 8.16 seconds
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos merged PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632839219
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +277,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
Review Comment:
maybe return the StringBuffer so this can be used fluently?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632842172
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -197,9 +197,11 @@ public JsopBuilder value(boolean value) {
* @param value the value
* @return this
*/
- @Override
public JsopBuilder value(String value) {
- return encodedValue(encode(value));
+ optionalCommaAndNewline(strLength(value));
Review Comment:
why do we loose the "@Override" 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.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632840377
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +277,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
+ if (s == null) {
+ return buff.append("null");
+ }
+ int length = s.length();
+ if (length == 0) {
+ return buff.append("\"\"");
+ }
+ for (int i = 0; i < length; i++) {
+ char c = s.charAt(i);
+ if (c == '\"' || c == '\\' || c < ' ' || (c >= 0xd800 && c <= 0xdbff)) {
Review Comment:
Maybe extract the logic that detects what needs to be escaped?
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632859435
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +277,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
+ if (s == null) {
+ return buff.append("null");
+ }
+ int length = s.length();
+ if (length == 0) {
+ return buff.append("\"\"");
+ }
+ for (int i = 0; i < length; i++) {
+ char c = s.charAt(i);
+ if (c == '\"' || c == '\\' || c < ' ' || (c >= 0xd800 && c <= 0xdbff)) {
Review Comment:
Done.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632861940
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +278,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
+ if (s == null) {
+ return buff.append("null");
+ }
+ int length = s.length();
+ if (length == 0) {
+ return buff.append("\"\"");
Review Comment:
I personally find it easier to *read*, when all returns are on the same level of nesting (so this would require else/if etc). But this may be a matter of taste.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632871987
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +278,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
+ if (s == null) {
+ return buff.append("null");
+ }
+ int length = s.length();
+ if (length == 0) {
+ return buff.append("\"\"");
Review Comment:
good point
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632858732
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +277,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
Review Comment:
The method does return the StringBuilder received as an argument.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632869189
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -275,6 +278,34 @@ public static String encode(String s) {
return '\"' + s + '\"';
}
+ /**
+ * Similar to #encode(String) but appends the result to the given buffer. This method exists to avoid the allocation
+ * of intermediate objects to represent the encoded String when the caller already has a buffer where the result
+ * should be appended.
+ *
+ * @param s the text to convert
+ * @param buff the target buffer
+ * @return the target buffer with the encoded string appended
+ */
+ private static StringBuilder encode(String s, StringBuilder buff) {
+ if (s == null) {
+ return buff.append("null");
+ }
+ int length = s.length();
+ if (length == 0) {
+ return buff.append("\"\"");
Review Comment:
As this method is a variation of `StringBuilder encode(String s)`, I kept it as close as possible to the other method with only the changes needed to use an existing `StringBuilder`, without doing any stylist changes, as this seems to be outside the scope of this PR.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10866 - Avoid creating an intermediate String when appending a string value to a JsopBuilder. [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1511:
URL: https://github.com/apache/jackrabbit-oak/pull/1511#discussion_r1632857995
##########
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/json/JsopBuilder.java:
##########
@@ -197,9 +197,11 @@ public JsopBuilder value(boolean value) {
* @param value the value
* @return this
*/
- @Override
public JsopBuilder value(String value) {
- return encodedValue(encode(value));
+ optionalCommaAndNewline(strLength(value));
Review Comment:
Missed that one. Fixed.
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org