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.

Sign up for GitHub

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

Jump to bottom

ICU-22761 Optimize get value of LocaleObjectCache #2984

Merged
merged 1 commit into from
Sep 23, 2024
Merged

ICU-22761 Optimize get value of LocaleObjectCache #2984

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions icu4j/main/core/src/main/java/com/ibm/icu/impl/locale/LocaleObjectCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,34 @@ public V get(K key) {
}
if (value == null) {
key = normalizeKey(key);
V newVal = createObject(key);
if (key == null || newVal == null) {
// subclass must return non-null key/value object
// subclass must return non-null key object
if (key == null) {
return null;
}

CacheEntry<K, V> newEntry = new CacheEntry<K, V>(key, newVal, _queue);
entry = _map.get(key);
if (entry != null) {
value = entry.get();
}
// hit cache
if (value != null) {
return value;
}

while (value == null) {
cleanStaleEntries();
entry = _map.putIfAbsent(key, newEntry);
if (entry == null) {
value = newVal;
break;
} else {
value = entry.get();
}
// if map not contains key or the referent value of CacheEntry is set to be null
// both need create a new value
V newVal = createObject(key);
if (newVal == null) {
// subclass must return non-null value object
return null;
}

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.

// clean recycled SoftReferences again
cleanStaleEntries();
return newVal;
}
return value;
}
Expand Down