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

ICU-22761 Optimize get value of LocaleObjectCache #2984

Merged

Conversation

imurluck
Copy link
Contributor

@imurluck imurluck commented Apr 29, 2024

When invoking get(K key) method of LocaleObjectCache, it may be blocked for a long time, looping in lines 45 and 53, because after the referent of SoftReference is set to NULL, it may take a long time to add SoftReference to its ReferenceQueue if GC is slowly.

For example, in the Concurrent Copying GC in Android Art virtual machine, the referent in SoftReference is set to null during the copying phase, while the SoftReference is added to its ReferenceQueue in the ReferenceQueueDaemon thread after GC, which experiences a long time in between

In fact, we have found many examples of blocking for more than 10s on some Android devices

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22761
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

imurluck added a commit to imurluck/icu that referenced this pull request Apr 29, 2024
@imurluck imurluck force-pushed the ICU-22761-locale-object-cache-blocked branch from 9131585 to b053f66 Compare April 29, 2024 13:14
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@imurluck
Copy link
Contributor Author

imurluck commented May 6, 2024

@markusicu markusicu force-pushed the ICU-22761-locale-object-cache-blocked branch from b053f66 to fb1c1f3 Compare September 22, 2024 21:31
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

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

IIUC:

  • The old code never replaces an existing Map value. If there is a CacheEntry with a null value, it waits until the existing CacheEntry goes away or has a non-null value.
  • The claim is that this can take a long time.
  • The new code fetches the CacheEntry once more after normalizing the key, and if it's not there or empty, it just stuffs the newly created value into the Map.

This seems plausible to me. It seems to operate properly on the Map, and by eliminating the loop it avoids any chance of taking a long time.

Anyone opposed?

@markusicu
Copy link
Member

FYI -- I rebased the branch onto current main in order to update the CI checks.

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

Looks ok to me, but I know caches are tricky. Too bad we can't use the Guava caches

@markusicu markusicu merged commit 1c33fcf into unicode-org:main Sep 23, 2024
14 checks passed

CacheEntry<K, V> newEntry = new CacheEntry<K, V>(key, newVal, _queue);
// just replace it
_map.put(key, newEntry);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a valid bug, but I just don't know concurrent code good enough to provide the best solution.

Does map.put cause a risk of creating duplicated objects used by the callers? I don't know if callers use == to compare objects, though arguably, we should fix the call sites if it exists, but if it happens, it would be difficult to debug such race if the race can't be reproduced.

@markusicu, Can ICU4J take advantage of the atomic operations provided by ConcurrentHashMap since Java 8?

I have a quick draft of a point fix for the performance issue by evicting the cache earlier if the cache stores a null value.
For line 47 - 53, it can be re-implemented with Java 8 API

        AtomicReference<Object> ref = new AtomicReference<>();
        _map.compute(key, (_, oldEntry) -> {
            if (oldEntry == null) {
                ref.set(newVal);
                return newEntry;
            }
            String oldVal = oldEntry.get();
            if (oldVal != null) {
                ref.set(oldVal);
                return oldEntry;
            } else {
                ref.set(newVal);
                return newEntry;
            }
        });
        value = ref.get();

Copy link
Member

Choose a reason for hiding this comment

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

Does map.put cause a risk of creating duplicated objects used by the callers?

I am pretty sure that there are race conditions here where different callers could end up using equivalent but different objects. I think that's probably ok, and in order to avoid any of these, I suspect that the whole get() function -- or a part that checks once more before it puts -- would have to be synchronized.

@markusicu, Can ICU4J take advantage of the atomic operations provided by ConcurrentHashMap since Java 8?

I assume so. We can use the intersection of Java 8 and some Android API level that includes much of Java 8. (I would have to check the download page to see which API level exactly.) In ICU, we don't actually test Android API level compatibility -- we would need help with that.

I have a quick draft of a point fix for the performance issue by evicting the cache earlier if the cache stores a null value. For line 47 - 53, it can be re-implemented with Java 8 API

I have merged this PR here. Feel free to propose a new one.

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

Successfully merging this pull request may close these issues.

5 participants