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

Optimize cache usage in DefaultConversionService and fix race condition #10143

Merged

Conversation

PakhomovAlexander
Copy link

@PakhomovAlexander PakhomovAlexander commented Nov 21, 2023

The usage of typeConverters and converterCache are optimized.

  1. Redundant operations are removed, which boosts the performance (according to the ConversionServiceBenchmark) at ~ 2x.

  2. The synchronized keyword is added to the critical methods that can be called from several threads. Now typeConverters and converterCache are consistent.

Here are the benchmark results that I've run on my Macbook 2,6 GHz 6-Core Intel Core i7, jdk-11.0.12.jdk:

Benchmark                                     Mode  Cnt         Score          Error  Units

Baseline:
ConversionServiceBenchmark.convertCacheHit   thrpt    5  27404123.384 ± 5692431.522  ops/s
ConversionServiceBenchmark.convertCacheMiss  thrpt    5   4923550.293 ±  461767.609  ops/s

Refactored (1):
ConversionServiceBenchmark.convertCacheHit   thrpt    5  48408342.853 ± 9369123.821  ops/s
ConversionServiceBenchmark.convertCacheMiss  thrpt    5   4842578.352 ±   34813.903  ops/s

Refactored + synchronized (1 + 2):
ConversionServiceBenchmark.convertCacheHit   thrpt    5  38918162.398 ± 10242478.879  ops/s
ConversionServiceBenchmark.convertCacheMiss  thrpt    5   4512350.735 ±   981537.261  ops/s

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2023

CLA assistant check
All committers have signed the CLA.

@PakhomovAlexander
Copy link
Author

Also, the original issue is not reproduced in this fix.

@wetted Do you think this fix is good enough?

@wetted wetted requested a review from yawkat November 21, 2023 16:40
@wetted
Copy link
Contributor

wetted commented Nov 21, 2023

@PakhomovAlexander thank you for the PR. I'm asking for reviews from those who know micronaut core better than I.

@wetted wetted linked an issue Nov 21, 2023 that may be closed by this pull request
@sdelamo
Copy link
Contributor

sdelamo commented Nov 22, 2023

in which scenario did you get the race condition. In tests?

@PakhomovAlexander
Copy link
Author

@sdelamo I've described the case with the race condition in this issue #10091

Here is the repo wich reproduces it https://github.com/wetted/core-10091-race-condition

TLDR; Yes, first I got the race condition on tests but then I managed to reproduce it in the main code. Basically, any application that starts context from different threads can face this issue.

@sdelamo sdelamo self-assigned this Dec 4, 2023
@PakhomovAlexander
Copy link
Author

Hi @sdelamo, could you please clarify the state of this PR? I wonder if the fix will appear in the upcoming 3.10.x release. Thanks!

@sdelamo sdelamo merged commit b611160 into micronaut-projects:3.10.x Jan 9, 2024
5 checks passed
@sdelamo
Copy link
Contributor

sdelamo commented Jan 9, 2024

@PakhomovAlexander it will be part of a 3.10.x release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Race condition on concurrent ApplicationContext start [3.10.x]
6 participants