GROOVY-9637: Improve the performance of GString

[daniellansun]

See https://issues.apache.org/jira/browse/GROOVY-9637

[paulk-asert]

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.

[daniellansun]

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

[paulk-asert]

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.

[blackdrag]

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

[daniellansun]

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.

[paulk-asert]

@blackdrag Did you have in mind a property/switch like we can turn on int optimisations for our classic bytecode generation?

[blackdrag]

@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.

[paulk-asert]

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.

[daniellansun]

Here is the test script. Groovy 3.0.5 costs about 8500ms, and this PR costs about 210ms.

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"

[daniellansun]

superseded by #1322