Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Thu, 17 Mar 2022 03:09:53 GMT, Stuart Marks wrote: > There's already a bug for this: > [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). This > includes creating a new API as well as fixing up a bunch of call sites. > There's a partial list of call sites in java.base there. Go ahead and open a > PR if you like. got it. > I'd avoid updating all the individual call sites with the actual computation > `(int) (Math.ceil(expectedSize/0.75))` because we might want to change it in > the future. Yes, I still prefer my int calculation way... But I guess a jmh for several implementations is necessary. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test There's already a bug for this: [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). This includes creating a new API as well as fixing up a bunch of call sites. There's a partial list of call sites in java.base there. Go ahead and open a PR if you like. I've added an initial suggestion at an API as a comment on that bug. There may be some bikeshedding about the API. If it gets too bad, a potential fallback position would be to create a JDK-internal utility method and call it instead, and sidestep the creation of a public API. (However, I do think we need a public API for this.) I'd avoid updating all the individual call sites with the actual computation `(int) (Math.ceil(expectedSize/0.75))` because we might want to change it in the future. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Wed, 16 Mar 2022 16:46:58 GMT, Stuart Marks wrote: > No, you don't need to do any rebasing; when the change is integrated, all > these commits will automatically be squashed into a single commit. If that > can't be done, the bot will detect it and give a warning, which will probably > include instructions. But basically you'd merge from the master branch and > commit the merge after resolving any conflicts. > > None of that needs to be done in this case, so I'll just go ahead and sponsor > this. Thanks. So the next step would be clean the usage of ` x / 0.75F+1F` in codes. Should I create one another report at https://bugreport.java.com/bugreport/ ? - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test No, you don't need to do any rebasing; when the change is integrated, all these commits will automatically be squashed into a single commit. If that can't be done, the bot will detect it and give a warning, which will probably include instructions. But basically you'd merge from the master branch and commit the merge after resolving any conflicts. None of that needs to be done in this case, so I'll just go ahead and sponsor this. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test > what I worried is, the boundary this is based on the current table size > calculating mechanic in HashMap. > If people change the mechanic in HashMap, then the boundary would change. > But well, this is a white box text for HashMap (and HashMap-like) classes > after all, so maybe I'm just over overthinking too much. Yes, the boundary conditions are sensitive to the exact calculation used for table sizing. If the calculation changes, the boundary results will most likely change, and the test will fail. But that's ok since this is a whitebox test. In any case, changes look good, and I've run this through our internal build/test system and the results are good. Ready to integrate! - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
> 8281631: HashMap copy constructor and putAll can over-allocate table XenoAmess has updated the pull request incrementally with one additional commit since the last revision: refine test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/51871859..a980bda4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7431=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7431=32-33 Stats: 350 lines in 1 file changed: 2 ins; 291 del; 57 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
