Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8288723: Avoid redundant ConcurrentHashMap.get call in java.time #9208

Conversation

turbanoff
Copy link
Member

@turbanoff turbanoff commented Jun 18, 2022

Instead of separate ConcurrentHashMap.get call, we can use result of previous putIfAbsent call.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8288723: Avoid redundant ConcurrentHashMap.get call in java.time

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9208/head:pull/9208
$ git checkout pull/9208

Update a local copy of the PR:
$ git checkout pull/9208
$ git pull https://git.openjdk.org/jdk pull/9208/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9208

View PR using the GUI difftool:
$ git pr show -t 9208

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9208.diff

Instead of separate ConcurrentHashMap.get call, we can use result of previous putIfAbsent call.
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 18, 2022

👋 Welcome back aturbanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jun 18, 2022

@turbanoff The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Jun 18, 2022
@turbanoff turbanoff changed the title [PATCH] Avoid redundant ConcurrentHashMap.get call in java.time 8288723: Avoid redundant ConcurrentHashMap.get call in java.time Jun 20, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 20, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 20, 2022

Webrevs

Comment on lines 312 to 319
Object store = CACHE.get(key);
if (store == null) {
store = createStore(field, locale);
CACHE.putIfAbsent(key, store);
store = CACHE.get(key);
Object prev = CACHE.putIfAbsent(key, store);
if (prev != null) {
store = prev;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do better here and use computeIfAbsent with createStore as its lambda. You could even change the signature of createStore to take Entry<TemporalField, Locale> as its parameter and then you could have this method be:

    private Object findStore(TemporalField field, Locale locale) {
        return CACHE.computeIfAbsent(createEntry(field, locale), this::createStore);
    }

    ...

    private Object createStore(Entry<TemporalField, Locale> entry) {
        ...
    }

This applies to most other changes you made, the one in ZoneOffset is the only one that's slightly different because there you have adjustment of ID_CACHE too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaves slightly different from the old initialization; the concurrent hash map blocks when the mapping function is run, just in case if non-blocking instantiation is what we want. If that's not a problem, I would prefer szegedi's suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liach advance apologies for nitpicking: ConcurrentHashMap doesn't in general block while the mapping function runs. It can block some concurrent updates, namely those that hash to the same bin and it won't block concurrent reads. I'm not saying the concern you raised isn't valid, just wanted to clarify that the situation is not nearly as grave as overall blocking; after all it ain't a Collections.synchronizedMap :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since putIfAbsent may block get calls, the blocking of computeIfAbsent is minuscule as long as the creation code is efficient enough. Just be careful that when writing the lambda for computeIfAbsent, preferably write static lambdas (that doesn't use this, instances, or local variables) so that one lambda can be reused over time than having to create a new one on each invocation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

use computeIfAbsent where lambda could be short and static
@@ -330,8 +330,10 @@ public static WeekFields of(DayOfWeek firstDayOfWeek, int minimalDaysInFirstWeek
WeekFields rules = CACHE.get(key);
if (rules == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you don't even need this if block and the preceding CACHE.get(key) and maybe it can be replaced with:

return CACHE.computeIfAbsent(key, ignore -> new WeekFields(firstDayOfWeek, minimalDaysInFirstWeek));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it will generate garbage: non-static lambda.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already generates some garbage as it does string concatenation for the key. Here's an idea: declare a record class for the key, record CacheKey(DayOfWeek firstDayOfWeek, int minimalDaysInFirstWeek). It will be more efficient than using strings for keys, and then you can have a static lambda.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@szegedi szegedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more thoughts.

@@ -330,8 +330,10 @@ public static WeekFields of(DayOfWeek firstDayOfWeek, int minimalDaysInFirstWeek
WeekFields rules = CACHE.get(key);
if (rules == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already generates some garbage as it does string concatenation for the key. Here's an idea: declare a record class for the key, record CacheKey(DayOfWeek firstDayOfWeek, int minimalDaysInFirstWeek). It will be more efficient than using strings for keys, and then you can have a static lambda.

store = CACHE.get(key);
}
return store;
return CACHE.computeIfAbsent(key, e -> createStore(e.getKey(), e.getValue()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If createStore can be static, the call site will only have to construct a constant lambda than having to pass this to construct a new instance on every call

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you really want to noodle this for performance, you can also store a this-bound lambda in a private final instance field, so then it's only created once too. I wouldn't put it past javac to do this, but I'd have to disassemble the bytecode of a little test program to see whether it's the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be some JMH tests to confirm the performance?
The value domain of the keys is pretty limited (7 * 7) max; and I'm not sure that the combination of creating a new record and the hashcode and equals methods would be faster than string concat of a constant and a single digit integer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RogerRiggs is you comment about DateTimeTextProvider.findStore or WeekFields.of?
There is no record creation here, in DateTimeTextProvider.findStore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RogerRiggs in my view, it's not about performance per se… I'll sometimes help along these kinds of small fix PRs (that are more likely than not driven by IntelliJ code refactoring suggestions) if I think they lead to more idiomatic code in the JDK with no drawbacks. I think there's value in periodically revisiting existing code and improve it to use new language and library construct that have become available since – people study JDK source code, and I think it's important they see as good examples in there as possible. I do like seeing an old example of hand-rolled compute-if-absent being replaced by an actual computeIfAbsent call. Similarly, I think using records as composite keys is in general a better practice over simulating a composite key by creating a string representation of it.

Yeah, it'll load additional code at runtime for that record's class, and yes, I'm aware records' hashCode/equals/toString are implemented by delegating to factory methods that heavily use method handle combinators. If records are meant as lightweight value-semantics composites, it should be okay to use them like this and if there are performance concerns with their use, then those concerns should be addressed by improving their performance. OTOH, MH combinators installed into constant call sites are well optimized by HotSpot these days… anyhow, a discussion for another day.

In this specific instance, the value domain of the keys is limited, but the number of method calls to WeekFields.of isn't. It could be benchmarked but if there's concerns, it's also okay to de-scope the record-as-key from this PR. I don't have strong feelings either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was about WeekFields.of(), I misplaced the comment.

@szegedi All good points about modernizing code...
One of the reasons to ask about specific performance data is to validate the general performance impact of using lambdas. In the case of WeekFields.of(), the lambda is passed on every call to computeIfAbsent even if the key is present and the lambda won't be used. WeekFields is way-way down the long tail of frequency of use and I expect that 99% of calls are for the same one or two combinations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @RogerRiggs that performance of JDK is very important. It was one of main motivation for removing .get call.
I'm not an expert in microbenchmarking, and it would require significant time/resources for me to investigate which is better.
So I decided to revert back to original proposal (without lambda and computeIfAbsent) in WeekFields. If you think that it's better to improve code, I think we can always fill a separate issue for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair.

@openjdk
Copy link

openjdk bot commented Jul 10, 2022

@turbanoff This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8288723: Avoid redundant ConcurrentHashMap.get call in java.time

Reviewed-by: attila, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 428 new commits pushed to the master branch:

  • eeb345a: 8290533: Remove G1ConcurrentMark::mark_in_bitmap(uint, HeapRegion*,oop)
  • 89458e3: 8288107: Auto-vectorization for integer min/max
  • 3d3e3df: 8290069: IGV: Highlight both graphs of difference in outline
  • 1c05507: 8289743: AArch64: Clean up patching logic
  • 011958d: 8290374: Shenandoah: Remove inaccurate comment on SBS::load_reference_barrier()
  • 984cd02: 8290707: runtime/cds/appcds/dynamicArchive/DynamicLambdaWithUseImplMethodHandle.java fails with "Can't find sun.hotspot.whitebox"
  • 4b4d352: 8264999: GeneralPath.lineTo() to itself produces jagged lines
  • 43c47b1: 8290534: Move MacroAssembler::verified_entry to C2_MacroAssembler on x86
  • 5425573: 8290496: riscv: Fix build warnings-as-errors with GCC 11
  • a3e07d9: Merge
  • ... and 418 more: https://git.openjdk.org/jdk/compare/47b86690b6672301aa46d4a7b9ced58d17047cc7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2022
revert 'computeIfAbsent' when it requires custom key class.
Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks

@turbanoff
Copy link
Member Author

Thank you for review!
/integrate

@openjdk
Copy link

openjdk bot commented Jul 21, 2022

Going to push as commit 52cc6cd.
Since your change was applied there have been 447 commits pushed to the master branch:

  • 3582fd9: 8290359: Ensure that all directory streams are closed in jdk.link
  • 53fc495: 8290316: Ensure that all directory streams are closed in java.base
  • db1e44c: 8290353: ModuleReader::list specification should suggest closing the returned stream
  • 2c73a1f: 8290324: Move atomic operations outside of os_xxx.hpp
  • e8975be: 8290746: ProblemList compiler/vectorization/TestAutoVecIntMinMax.java
  • 9c19d89: Merge
  • 17e65bb: 8290625: Test jdk/javadoc/tool/CheckManPageOptions.java after manpage update
  • 618f3a8: 8278274: Update nroff pages in JDK 19 before RC
  • f1001a0: 8287916: Address the inconsistency between the constant array and pool size
  • 5d1c448: 8285407: Improve Xalan supports
  • ... and 437 more: https://git.openjdk.org/jdk/compare/47b86690b6672301aa46d4a7b9ced58d17047cc7...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 21, 2022
@openjdk openjdk bot closed this Jul 21, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 21, 2022
@openjdk
Copy link

openjdk bot commented Jul 21, 2022

@turbanoff Pushed as commit 52cc6cd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@turbanoff turbanoff deleted the avoid_redundant_ConcurrentHashMap.get_after_putIfAbsent_java.time branch October 27, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org integrated Pull request has been integrated
5 participants