8252827: Caching Integer.toString just like Integer.valueOf
Hello, does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed. Greetings Raffaello ---- [1] https://bugs.openjdk.java.net/browse/JDK-8252827
Hi, Is there any way to quantify the savings? And what technique can be applied to limit the size of the cache. The size of the small integer cache is somewhat arbitrary. Regards, Roger On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
I guess the reporter meant to limit the cache range similarly to the one used for valueOf(). I have no clue about the benefit/cost ratio for the proposed String cache. It really depends on usage, workload, etc. One can easily imagine both extreme scenarios but it's hard to tell how the average one would look. My post is only about either solving the issue by implementing the cache, which I can contribute to; or closing it because of lack of real-world need or interest. Greetings Raffaello On 2021-04-16 20:36, Roger Riggs wrote:
Hi,
Is there any way to quantify the savings? And what technique can be applied to limit the size of the cache. The size of the small integer cache is somewhat arbitrary.
Regards, Roger
On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
On 17/04/2021 4:54 am, Raffaello Giulietti wrote:
I guess the reporter meant to limit the cache range similarly to the one used for valueOf().
I have no clue about the benefit/cost ratio for the proposed String cache. It really depends on usage, workload, etc. One can easily imagine both extreme scenarios but it's hard to tell how the average one would look.
My post is only about either solving the issue by implementing the cache, which I can contribute to; or closing it because of lack of real-world need or interest.
Caching for the sake of caching is not an objective in itself. Unless the caching can be shown to solve a real problem, and the strategy for managing the cache is well-defined, then I would just close the enhancement request. (Historically whether an issue we don't have any firm plans to address is just left open "forever" or closed, depends very much on who does the bug triaging in that area. :) ) Cheers, David
Greetings Raffaello
On 2021-04-16 20:36, Roger Riggs wrote:
Hi,
Is there any way to quantify the savings? And what technique can be applied to limit the size of the cache. The size of the small integer cache is somewhat arbitrary.
Regards, Roger
On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
On 2021-04-17 07:07, David Holmes wrote:
On 17/04/2021 4:54 am, Raffaello Giulietti wrote:
I guess the reporter meant to limit the cache range similarly to the one used for valueOf().
I have no clue about the benefit/cost ratio for the proposed String cache. It really depends on usage, workload, etc. One can easily imagine both extreme scenarios but it's hard to tell how the average one would look.
My post is only about either solving the issue by implementing the cache, which I can contribute to; or closing it because of lack of real-world need or interest.
Caching for the sake of caching is not an objective in itself. Unless the caching can be shown to solve a real problem, and the strategy for managing the cache is well-defined, then I would just close the enhancement request. (Historically whether an issue we don't have any firm plans to address is just left open "forever" or closed, depends very much on who does the bug triaging in that area. :) )
Cheers, David
Indeed, the impression is that many of the issues are probably open because it's unclear whether they should be addressed with some implementation or spec effort or whether they should be closed. Triaging is certainly a harder job than it appears at first sight ;-) It would be useful to have a kind of "suspended" or "limbo" resolution state on the JBS for issues like this one, so people searching for more compelling open ones would not encounter them. Personally, I would close this issue without a "fix"; or "suspend" it. Greetings Raffaello
Greetings Raffaello
On 2021-04-16 20:36, Roger Riggs wrote:
Hi,
Is there any way to quantify the savings? And what technique can be applied to limit the size of the cache. The size of the small integer cache is somewhat arbitrary.
Regards, Roger
On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
Hi, I read the JBS bug and I interpret it as: - IntegerCache provides Integer instances for [-128, 127] by default - Having Integer.toString(int) could behave the same or at least cache most probable values like [-1 to 16] or using the IntegerCache range. It looks trivial and potentially could benefits to jdk itself to reduce memory garbage : is Integer.toString(int) widely used in the jdk codebase ? Finally it can be alternatively implemented in application's code. My 2 cents, Laurent Le sam. 17 avr. 2021 à 11:06, Raffaello Giulietti < raffaello.giulietti@gmail.com> a écrit :
On 2021-04-17 07:07, David Holmes wrote:
On 17/04/2021 4:54 am, Raffaello Giulietti wrote:
I guess the reporter meant to limit the cache range similarly to the one used for valueOf().
I have no clue about the benefit/cost ratio for the proposed String cache. It really depends on usage, workload, etc. One can easily imagine both extreme scenarios but it's hard to tell how the average one would look.
My post is only about either solving the issue by implementing the cache, which I can contribute to; or closing it because of lack of real-world need or interest.
Caching for the sake of caching is not an objective in itself. Unless the caching can be shown to solve a real problem, and the strategy for managing the cache is well-defined, then I would just close the enhancement request. (Historically whether an issue we don't have any firm plans to address is just left open "forever" or closed, depends very much on who does the bug triaging in that area. :) )
Cheers, David
Indeed, the impression is that many of the issues are probably open because it's unclear whether they should be addressed with some implementation or spec effort or whether they should be closed. Triaging is certainly a harder job than it appears at first sight ;-)
It would be useful to have a kind of "suspended" or "limbo" resolution state on the JBS for issues like this one, so people searching for more compelling open ones would not encounter them.
Personally, I would close this issue without a "fix"; or "suspend" it.
Greetings Raffaello
Greetings Raffaello
On 2021-04-16 20:36, Roger Riggs wrote:
Hi,
Is there any way to quantify the savings? And what technique can be applied to limit the size of the cache. The size of the small integer cache is somewhat arbitrary.
Regards, Roger
On 4/16/21 12:48 PM, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
Hi, in view of Integer becoming a primitive class [1], the IntegerCache is probably going to disappear. For a small, fixed range like the one you are proposing [-1, 16], there's no real need for a separate cache class. You could have a switch in the implementation of toString(), with the string literals then being part of the constant pool of the class. Not free beer, but supported by the VM since day 0. It's only when the range is open that you'd need a cache similar to IntegerCache. My 2 cents as well :-) Raffaello ---- [1] https://openjdk.java.net/jeps/402 On 2021-04-17 11:18, Laurent Bourgès wrote:
Hi,
I read the JBS bug and I interpret it as: - IntegerCache provides Integer instances for [-128, 127] by default - Having Integer.toString(int) could behave the same or at least cache most probable values like [-1 to 16] or using the IntegerCache range.
It looks trivial and potentially could benefits to jdk itself to reduce memory garbage : is Integer.toString(int) widely used in the jdk codebase ?
Finally it can be alternatively implemented in application's code.
My 2 cents, Laurent
Le sam. 17 avr. 2021 à 11:06, Raffaello Giulietti <raffaello.giulietti@gmail.com <mailto:raffaello.giulietti@gmail.com>> a écrit :
On 2021-04-17 07:07, David Holmes wrote: > On 17/04/2021 4:54 am, Raffaello Giulietti wrote: >> I guess the reporter meant to limit the cache range similarly to the >> one used for valueOf(). >> >> I have no clue about the benefit/cost ratio for the proposed String >> cache. It really depends on usage, workload, etc. One can easily >> imagine both extreme scenarios but it's hard to tell how the average >> one would look. >> >> My post is only about either solving the issue by implementing the >> cache, which I can contribute to; or closing it because of lack of >> real-world need or interest. > > Caching for the sake of caching is not an objective in itself. Unless > the caching can be shown to solve a real problem, and the strategy for > managing the cache is well-defined, then I would just close the > enhancement request. (Historically whether an issue we don't have any > firm plans to address is just left open "forever" or closed, depends > very much on who does the bug triaging in that area. :) ) > > Cheers, > David >
Indeed, the impression is that many of the issues are probably open because it's unclear whether they should be addressed with some implementation or spec effort or whether they should be closed. Triaging is certainly a harder job than it appears at first sight ;-)
It would be useful to have a kind of "suspended" or "limbo" resolution state on the JBS for issues like this one, so people searching for more compelling open ones would not encounter them.
Personally, I would close this issue without a "fix"; or "suspend" it.
Greetings Raffaello
>> >> Greetings >> Raffaello >> >> >> On 2021-04-16 20:36, Roger Riggs wrote: >>> Hi, >>> >>> Is there any way to quantify the savings? >>> And what technique can be applied to limit the size of the cache. >>> The size of the small integer cache is somewhat arbitrary. >>> >>> Regards, Roger >>> >>> On 4/16/21 12:48 PM, Raffaello Giulietti wrote: >>>> Hello, >>>> >>>> does the enhancement proposed in [1] make sense, both today and when >>>> wrappers will be migrated to primitive classes? >>>> If so, it should be rather simple to add it and I could prepare a PR. >>>> If not, the issue might perhaps be closed. >>>> >>>> >>>> Greetings >>>> Raffaello >>>> >>>> ---- >>>> >>>> [1] https://bugs.openjdk.java.net/browse/JDK-8252827 <https://bugs.openjdk.java.net/browse/JDK-8252827> >>>
(Catching up on old email threads.) I don't have much to add to the technical discussion here. Yes, caching the Integer instances seems obsolescent, and it seems unlikely that caching String conversions will be helpful. I've gone ahead and closed out the bug. [1] On triaging bugs... we do triage bugs pretty effectively, I think. However, enhancement requests (like this one) tend to languish. People tend to notice ones that are obviously bad ideas or obviously good ideas and act on them quickly. However, there are a lot of items that are neither obviously good nor bad, so they just sit there. Sometimes they just sit there even after there has been some email discussion on them. :-) Sometimes I think we take these requests a bit too seriously. It looks to me like the submitter put about five minutes of thought into the request, and we've collectively probably spent 10x that dealing with it. I probably put too much effort into this bug myself; instead of researching the history, I could have simply closed it with a comment saying, "Closed, per discussion in <core-libs-dev link>" which would have been reasonable. Anyway, it's closed. s'marks [1] https://bugs.openjdk.java.net/browse/JDK-8252827 On 4/17/21 5:19 AM, Raffaello Giulietti wrote:
Hi,
in view of Integer becoming a primitive class [1], the IntegerCache is probably going to disappear.
For a small, fixed range like the one you are proposing [-1, 16], there's no real need for a separate cache class. You could have a switch in the implementation of toString(), with the string literals then being part of the constant pool of the class. Not free beer, but supported by the VM since day 0.
It's only when the range is open that you'd need a cache similar to IntegerCache.
My 2 cents as well :-) Raffaello
----
[1] https://openjdk.java.net/jeps/402
On 2021-04-17 11:18, Laurent Bourgès wrote:
Hi,
I read the JBS bug and I interpret it as: - IntegerCache provides Integer instances for [-128, 127] by default - Having Integer.toString(int) could behave the same or at least cache most probable values like [-1 to 16] or using the IntegerCache range.
It looks trivial and potentially could benefits to jdk itself to reduce memory garbage : is Integer.toString(int) widely used in the jdk codebase ?
Finally it can be alternatively implemented in application's code.
My 2 cents, Laurent
Le sam. 17 avr. 2021 à 11:06, Raffaello Giulietti <raffaello.giulietti@gmail.com <mailto:raffaello.giulietti@gmail.com>> a écrit :
On 2021-04-17 07:07, David Holmes wrote: > On 17/04/2021 4:54 am, Raffaello Giulietti wrote: >> I guess the reporter meant to limit the cache range similarly to the >> one used for valueOf(). >> >> I have no clue about the benefit/cost ratio for the proposed String >> cache. It really depends on usage, workload, etc. One can easily >> imagine both extreme scenarios but it's hard to tell how the average >> one would look. >> >> My post is only about either solving the issue by implementing the >> cache, which I can contribute to; or closing it because of lack of >> real-world need or interest. > > Caching for the sake of caching is not an objective in itself. Unless > the caching can be shown to solve a real problem, and the strategy for > managing the cache is well-defined, then I would just close the > enhancement request. (Historically whether an issue we don't have any > firm plans to address is just left open "forever" or closed, depends > very much on who does the bug triaging in that area. :) ) > > Cheers, > David >
Indeed, the impression is that many of the issues are probably open because it's unclear whether they should be addressed with some implementation or spec effort or whether they should be closed. Triaging is certainly a harder job than it appears at first sight ;-)
It would be useful to have a kind of "suspended" or "limbo" resolution state on the JBS for issues like this one, so people searching for more compelling open ones would not encounter them.
Personally, I would close this issue without a "fix"; or "suspend" it.
Greetings Raffaello
>> >> Greetings >> Raffaello >> >> >> On 2021-04-16 20:36, Roger Riggs wrote: >>> Hi, >>> >>> Is there any way to quantify the savings? >>> And what technique can be applied to limit the size of the cache. >>> The size of the small integer cache is somewhat arbitrary. >>> >>> Regards, Roger >>> >>> On 4/16/21 12:48 PM, Raffaello Giulietti wrote: >>>> Hello, >>>> >>>> does the enhancement proposed in [1] make sense, both today and when >>>> wrappers will be migrated to primitive classes? >>>> If so, it should be rather simple to add it and I could prepare a PR. >>>> If not, the issue might perhaps be closed. >>>> >>>> >>>> Greetings >>>> Raffaello >>>> >>>> ---- >>>> >>>> [1] https://bugs.openjdk.java.net/browse/JDK-8252827 <https://bugs.openjdk.java.net/browse/JDK-8252827> >>>
Hello! I would vote to close this. The benefit for all JDK users is questionable. If it's necessary in a particular application, it could be implemented in user code. With best regards, Tagir Valeev. пт, 16 апр. 2021 г., 23:49 Raffaello Giulietti < raffaello.giulietti@gmail.com>:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
Ciao Raffaello, another general consideration: caching sometimes makes certain optimizations harder to pull off. Few years ago I've seen this talk at Fosdem: https://archive.fosdem.org/2020/schedule/event/reducing_gc_times/ See minute 5:53 - where it shows that cached Integer values (as per Integer::valueOf) inhibit scalarization, and using `new Integer(...)` leads to superior performances. It's a pattern I have observed several time when working with the Foreign Memory API, where most of the abstractions are immutable: attempting to cache things (e.g. MemoryAddress instances) often comes up with the same GC overhead (because these instances are escape analyzed away) and ends up with worse performance overall (loss of scalarization). Of course this is not _always_ the case - but I wanted to add this particular angle to the discussion. Cheers Maurizio On 16/04/2021 17:48, Raffaello Giulietti wrote:
Hello,
does the enhancement proposed in [1] make sense, both today and when wrappers will be migrated to primitive classes? If so, it should be rather simple to add it and I could prepare a PR. If not, the issue might perhaps be closed.
Greetings Raffaello
----
participants (7)
-
David Holmes
-
Laurent Bourgès
-
Maurizio Cimadamore
-
Raffaello Giulietti
-
Roger Riggs
-
Stuart Marks
-
Tagir Valeev