5 ms·

The original behavior, where a substring pins the full original string, is also how Go's substring slicing works. I much prefer this behavior, as it lets you p

by nteon 13y ago

The original behavior, where a substring pins the full original string, is also how Go's substring slicing works. I much prefer this behavior, as it lets you profile and add a copy yourself if you find yourself pinning large strings. The Java change to do a copy for every substring seems like it harms the general case to help out poorly reasoned code. While the tracking of intervals with a tree is certainly impressive, it is a lot of complexity to again (in my opinion) support poorly written code. If you know you are going to only use 10 bytes of a 1 MB incoming HTTP request, make a copy.

jug6ernaut 13y ago
While you are not wrong about this all being to accommodate for badly written code, I don't think that a programmer should specifically have to adapt there code for something as simple as substring() to accommodate for a flaw in the language implementation(memory leak).
- emn13 13y ago
  It's not a flaw - nowhere is it specified that the substring doesn't retain a reference to the original string; nor in any case does the garbage collecter make very many guarantees in the first place. The new behaviour is fine. But it's not a bugfix, and it causes nasty problems - probably causes more problems than it fixes simply because it's a change in behavior. In order to write efficient string-processing algorithms in java, since the docs habitually don't specify efficiency, you need to make assumptions about which operations are how fast. Making an operation this much slower - and it's really a huge difference - is pretty risky. Also, note that memory use in the new model is also likely to be higher, and can even be catastrophically higher (e.g. a recursive descent parser might retain references to all suffixes it builds during descent, causing quadratic memory usage). I'm sure there are good reasons for the change, but the the way it was released is really shoddy - hiding it away in a minor (ideally just bugfix+security) release without a good warning well in advance means they didn't consider this to be breaking change, and that's just being ridiculously naive for a project as mature as the JVM.
ape4 13y ago
I think the best answer is to take the size of the parent and child strings into account. If the parent is 1M and the child is just a few bytes then make a copy so the parent can be freed up. Otherwise use the old behavior.
- boomzilla 13y ago
  Yeah, I don't understand why the JDK dev did not take this approach. Even better, they can add a new API that allows developers specify the implementation that fits better. Something like String.substr(..., boolean allocateSpace) that defaults to the original behavior of substr.
- enjo 13y ago
  That'd be even worse. Now I have yet another variable to deal with when tracking down performance issues in my code. One that would be non-obvious to anyone not terribly familiar with how things are working behind the scenes. Better would be to introduce a methods that explicitly exposes the type of substring method being used.
  - ape4 13y ago
    But you wouldn't have to track down performance issues in your code if it never was slow ;)
benjiweber 13y ago
It previously wasn't clear that is how substring operated. In fact the Javadoc didn't even mention it. "new string" is rather misleading here. http://docs.oracle.com/javase/6/docs/api/java/lang/String.html#substring%28int%29 http://docs.oracle.com/javase/6/docs/api/java/lang/String.ht... Not documenting it in the Javadoc kind of makes sense since it is an implementation specific detail whether substring returns a view onto the original char array or not. However, it is certainly surprising that what is apparently a 2 character string could be taking up gigabytes of ram. It's the kind of behaviour that it would be useful to express and be able to choose via the API. e.g. make people write .substring(n).view() or .substring(n).copy() or similar.
- Someone 13y ago
  "new string" is rather misleading here. But the string _is_ new; there was no object that was Object.Equals to it before the call. That new string just never forgets where it came from. e.g. make people write .substring(n).view() or .substring(n).copy() or similar. That's what people have been doing for years: .substring(n) versus new String(substring(n)) (and, as a third option: .substring(n).intern()) I find it weird that this change is made because, as a side-effect, they almost have to start optimizing "new String(s)" calls. With this change, the only thing that can be useful for is for creating a String that is the same as another String, but not equal to it. I think that aspect of that call is rarely, if ever, used. If one can proof that it is never used, the next step on this path would be to make that call into a no-op. If one cannot proof it, it still would likely be a good speed up, but it also would be extremely risky.

by nteon 13y ago

jug6ernaut 13y ago

While you are not wrong about this all being to accommodate for badly written code, I don't think that a programmer should specifically have to adapt there code for something as simple as substring() to accommodate for a flaw in the language implementation(memory leak).

emn13 13y ago
It's not a flaw - nowhere is it specified that the substring doesn't retain a reference to the original string; nor in any case does the garbage collecter make very many guarantees in the first place. The new behaviour is fine. But it's not a bugfix, and it causes nasty problems - probably causes more problems than it fixes simply because it's a change in behavior. In order to write efficient string-processing algorithms in java, since the docs habitually don't specify efficiency, you need to make assumptions about which operations are how fast. Making an operation this much slower - and it's really a huge difference - is pretty risky. Also, note that memory use in the new model is also likely to be higher, and can even be catastrophically higher (e.g. a recursive descent parser might retain references to all suffixes it builds during descent, causing quadratic memory usage). I'm sure there are good reasons for the change, but the the way it was released is really shoddy - hiding it away in a minor (ideally just bugfix+security) release without a good warning well in advance means they didn't consider this to be breaking change, and that's just being ridiculously naive for a project as mature as the JVM.

ape4 13y ago

I think the best answer is to take the size of the parent and child strings into account. If the parent is 1M and the child is just a few bytes then make a copy so the parent can be freed up. Otherwise use the old behavior.

boomzilla 13y ago
Yeah, I don't understand why the JDK dev did not take this approach. Even better, they can add a new API that allows developers specify the implementation that fits better. Something like String.substr(..., boolean allocateSpace) that defaults to the original behavior of substr.
enjo 13y ago
That'd be even worse. Now I have yet another variable to deal with when tracking down performance issues in my code. One that would be non-obvious to anyone not terribly familiar with how things are working behind the scenes. Better would be to introduce a methods that explicitly exposes the type of substring method being used.
- ape4 13y ago
  But you wouldn't have to track down performance issues in your code if it never was slow ;)

benjiweber 13y ago

It previously wasn't clear that is how substring operated. In fact the Javadoc didn't even mention it. "new string" is rather misleading here. http://docs.oracle.com/javase/6/docs/api/java/lang/String.html#substring%28int%29 http://docs.oracle.com/javase/6/docs/api/java/lang/String.ht... Not documenting it in the Javadoc kind of makes sense since it is an implementation specific detail whether substring returns a view onto the original char array or not. However, it is certainly surprising that what is apparently a 2 character string could be taking up gigabytes of ram. It's the kind of behaviour that it would be useful to express and be able to choose via the API. e.g. make people write .substring(n).view() or .substring(n).copy() or similar.

Someone 13y ago
"new string" is rather misleading here. But the string _is_ new; there was no object that was Object.Equals to it before the call. That new string just never forgets where it came from. e.g. make people write .substring(n).view() or .substring(n).copy() or similar. That's what people have been doing for years: .substring(n) versus new String(substring(n)) (and, as a third option: .substring(n).intern()) I find it weird that this change is made because, as a side-effect, they almost have to start optimizing "new String(s)" calls. With this change, the only thing that can be useful for is for creating a String that is the same as another String, but not equal to it. I think that aspect of that call is rarely, if ever, used. If one can proof that it is never used, the next step on this path would be to make that call into a no-op. If one cannot proof it, it still would likely be a good speed up, but it also would be extremely risky.