You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/07/12 16:48:12 UTC
[GitHub] [groovy] danielsun1106 opened a new pull request #1309: GROOVY-9637: Improve the performance of GString
danielsun1106 opened a new pull request #1309:
URL: https://github.com/apache/groovy/pull/1309
See https://issues.apache.org/jira/browse/GROOVY-9637
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660591379
STC supports mutability of `GString` too, so it would be a breaking change anyway. If some user changes the strings/values via the bad practice and is impacted by the change, creating a new `GString` instance could be a workaround.
```
@groovy.transform.CompileStatic
def x() {
def s = "a${1}b"
s.strings[0] = 'c'
s.values[0] = 2
s
}
x() // yields c2b
```
----------------------------------------------------------------
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] [groovy] paulk-asert commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660605634
I understood it would remain a breaking change but users would have an easy and known approach to getting back the original behavior, i.e. go dynamic, and those wanting more performance would have a known approach for getting that - create the GString in a @CS method or as a field of a @CS class. Might be worth discussing on the mailing list.
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on a change in pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#discussion_r453374407
##########
File path: src/main/java/groovy/lang/GString.java
##########
@@ -276,4 +309,16 @@ public Pattern negate() {
public byte[] getBytes(String charset) throws UnsupportedEncodingException {
return toString().getBytes(charset);
}
+
+ private boolean checkValuesImmutable() {
+ for (Object value : values) {
+ if (null == value) continue;
+ if (!(IMMUTABLE_TYPE_LIST.contains(value.getClass())
Review comment:
enum's `toString` may return different string literal for each time they are called, e.g.
```groovy
enum Color {
RED, GREEN, BLUE
String toString() {
return "${new Date()}"
}
}
println Color.RED
```
So we can not account for enum.
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662822845
Here is the test script. Groovy 3.0.5 costs about 8500ms, and this PR costs about 210ms.
```groovy
long b = System.currentTimeMillis()
def gstr = "integer: ${1}, double: ${1.2d}, string: ${'x'}, class: ${Map.class}, boolean: ${true}"
for (int i = 0; i < 10000000; i++) {
gstr.toString()
}
long e = System.currentTimeMillis()
println "${e - b}ms"
```
----------------------------------------------------------------
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] [groovy] danielsun1106 closed pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 closed pull request #1309:
URL: https://github.com/apache/groovy/pull/1309
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662834699
superseded by https://github.com/apache/groovy/pull/1322
----------------------------------------------------------------
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] [groovy] danielsun1106 edited a comment on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 edited a comment on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660591379
STC supports mutability of `GString` too, so it would be a breaking change anyway. If some user changes the strings/values via the bad practice and is impacted by the change, creating a new `GString` instance could be a workaround.
```groovy
@groovy.transform.CompileStatic
def x() {
def s = "a${1}b"
s.strings[0] = 'c'
s.values[0] = 2
s
}
x() // yields c2b
```
----------------------------------------------------------------
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] [groovy] paulk-asert commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660422223
I still wonder whether some folks might make use of current GString mutability in scenarios like templating. I wonder whether just applying the optimisations when @CompileStatic is in play makes sense. In that scenario, we could leave GString and GStringImpl classes alone and have a GStringImplCS class.
----------------------------------------------------------------
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] [groovy] blackdrag commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
blackdrag commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662355159
@paulk-asert @danielsun1106 now you guys forced me to look into this in much more detail and thought. The current implementation in this PR will build the string potentially only once and then this will be the final result. The part with clone I did originally not even see (I think I looked at it before you did that change), also does not improve performance. But you need it to ensure the output string is always the same. But I think this is all thought a bit to hasty/short, because you have no control over the subclasses of GString, which might be another class than GStringimpl:
* breaking change: subclass may override getValues and expect changes from there taken into account for the final string. Since you are now using this.value, this cannot happen anymore
* breaking change: you now assume all subclasses of GString will be GStringImpl by imposing the restriction on getStrings. But nothing can ensure this right now
* breaking change: classes subclassing GStringImpl and overriding getStrings not returning the same strings will be ignored in GString
* breaking change: calling getValues and then changing an element of the array no longer takes affect
* breaking change: calling getStrings and then changing an element of the array no longer takes affect, if the implementing class is GStringImpl
If we really wanted to go in this direction, then I would do the following:
* merge GString and GStringImpl, since the implementation split with unchecked restrictions makes no sense for more than one possible subclass
* make getValues and getStrings final or the class itself, since it makes no sense for classes to change that anymore
* add a method to change the behaviour of GString between calculating once and every time. (property could be used to control the default)
But actually I am unsure about this last point... because if you change the behaviour it works fine with mutable classes, for which this PR already caters. So it is values/strings set from outside that really matter here, and the PR does actually not allow them being set, because of the methods returning a cloned array.
Which means for me, that the current version of this PR is quite a stripped down version of what it was before and more a structured string, than what it was before. Not that this is bad in itself, it just has other use cases than GString. If all the old use cases of GString are important to us, we cannot do this here like this. If not, well, then we have a major breaking change.
We could of course also do this slightly different. Let's say:
* keep getValues and getStrings return the cloned array
* add mutator methods for values and strings which will reset the output string
This is still a breaking change of course and especially the part with changing array elements of getValues being ignored is not nice, since it will be a silent bug. Instead I would think of these methods returning a list instead and make that immutable, which allows for better error messages.
----------------------------------------------------------------
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] [groovy] paulk-asert commented on a change in pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#discussion_r453359180
##########
File path: src/main/java/groovy/lang/GString.java
##########
@@ -276,4 +309,16 @@ public Pattern negate() {
public byte[] getBytes(String charset) throws UnsupportedEncodingException {
return toString().getBytes(charset);
}
+
+ private boolean checkValuesImmutable() {
+ for (Object value : values) {
+ if (null == value) continue;
+ if (!(IMMUTABLE_TYPE_LIST.contains(value.getClass())
Review comment:
Perhaps we could use ImmutablePropertyUtils#isBuiltinImmutable? We'd have to account for primitives. Also check for enums?
----------------------------------------------------------------
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] [groovy] paulk-asert edited a comment on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662237401
@blackdrag Did you have in mind a property/switch like we can turn on int optimisations for our classic bytecode generation?
----------------------------------------------------------------
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] [groovy] blackdrag commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
blackdrag commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660618512
How about adding some kind of makeMutable/makeImmutabel methods on GString to control this (please find a better name)? controling this by @cs I find a bit dangerous
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660621265
Let me clarify a bit more, the **immutable** we discuss means the arrays `strings` and `values` stored in `GString` instance are immutable, which is what the PR has done.
Without applying the PR, they are both mutable because they are returned as they are, i.e. not returning their copy.
----------------------------------------------------------------
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] [groovy] danielsun1106 edited a comment on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 edited a comment on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-660591379
STC supports mutability of `GString` too, so it would be a breaking change anyway. If some user changes the strings/values via the **bad practice** and is impacted by the change, creating a new `GString` instance could be a workaround.
```groovy
@groovy.transform.CompileStatic
def x() {
def s = "a${1}b"
s.strings[0] = 'c'
s.values[0] = 2
s
}
x() // yields c2b
```
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on a change in pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#discussion_r453376222
##########
File path: src/main/java/groovy/lang/GString.java
##########
@@ -276,4 +309,16 @@ public Pattern negate() {
public byte[] getBytes(String charset) throws UnsupportedEncodingException {
return toString().getBytes(charset);
}
+
+ private boolean checkValuesImmutable() {
+ for (Object value : values) {
+ if (null == value) continue;
+ if (!(IMMUTABLE_TYPE_LIST.contains(value.getClass())
Review comment:
Tweaked according to your suggestion:
https://github.com/apache/groovy/pull/1309/commits/6187f3e4355db563a3d39d4698ec1b2b064b3406
----------------------------------------------------------------
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] [groovy] paulk-asert commented on a change in pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#discussion_r453374833
##########
File path: src/main/java/groovy/lang/GString.java
##########
@@ -276,4 +309,16 @@ public Pattern negate() {
public byte[] getBytes(String charset) throws UnsupportedEncodingException {
return toString().getBytes(charset);
}
+
+ private boolean checkValuesImmutable() {
+ for (Object value : values) {
+ if (null == value) continue;
+ if (!(IMMUTABLE_TYPE_LIST.contains(value.getClass())
Review comment:
Ah yes, I hadn't thought about that case.
----------------------------------------------------------------
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] [groovy] paulk-asert edited a comment on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662237401
@blackdrag Did you have in mind a property/switch like we turn on int optimisations for our classic bytecode generation?
----------------------------------------------------------------
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] [groovy] paulk-asert commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662814220
Your latest changes provide a consistent design but are a complete breaking change. This might end up being the way to go but let's hold off for a little while. I'd like to ponder alternatives which give us a better binary compatibility story.
----------------------------------------------------------------
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] [groovy] danielsun1106 commented on a change in pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#discussion_r453374407
##########
File path: src/main/java/groovy/lang/GString.java
##########
@@ -276,4 +309,16 @@ public Pattern negate() {
public byte[] getBytes(String charset) throws UnsupportedEncodingException {
return toString().getBytes(charset);
}
+
+ private boolean checkValuesImmutable() {
+ for (Object value : values) {
+ if (null == value) continue;
+ if (!(IMMUTABLE_TYPE_LIST.contains(value.getClass())
Review comment:
enum's `toString` may return different string literal for each time they are called, e.g.
```groovy
enum Color {
RED, GREEN, BLUE
String toString() {
return "${new Date()}"
}
}
println Color.RED
```
----------------------------------------------------------------
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] [groovy] paulk-asert commented on pull request #1309: GROOVY-9637: Improve the performance of GString
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1309:
URL: https://github.com/apache/groovy/pull/1309#issuecomment-662237401
@blackdrag Did you have in mind a property/switch like we turn on int optimisations?
----------------------------------------------------------------
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