Skip to content

Various simplifications to code and comments. #6015

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

Closed
copybara-service wants to merge 1 commit into from
Closed

Various simplifications to code and comments. #6015

Show file tree
Hide file tree
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
29 changes: 16 additions & 13 deletions android/guava/src/com/google/common/cache/CacheBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@
* <li>accumulation of cache access statistics
* </ul>
*
* <p>These features are all optional; caches can be created using all or none of them. By default
* <p>These features are all optional; caches can be created using all or none of them. By default,
* cache instances created by {@code CacheBuilder} will not perform any type of eviction.
*
* <p>Usage example:
Expand Down Expand Up @@ -231,13 +231,16 @@ public CacheStats snapshot() {
});
static final CacheStats EMPTY_STATS = new CacheStats(0, 0, 0, 0, 0, 0);

static final Supplier<StatsCounter> CACHE_STATS_COUNTER =
new Supplier<StatsCounter>() {
@Override
public StatsCounter get() {
return new SimpleStatsCounter();
}
};
/*
* We avoid using a method reference here for now: Inside Google, CacheBuilder is used from the
* implementation of a custom ClassLoader that is sometimes used as a system classloader. That's a
* problem because method-reference linking tries to look up the system classloader, and it fails
Comment on lines 231 to +237
Copy link

@BitoAgent BitoAgent Mar 25, 2024

Choose a reason for hiding this comment

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

Optimization Issue: Utilizing diamond syntax for the instantiation of generic classes such as ConcurrentLinkedQueue enhances code readability and conciseness. This change leverages the compiler's ability to infer type parameters, reducing redundancy and improving the overall clarity of the code. It's a straightforward yet effective way to maintain cleaner code, adhering to modern Java development practices.
Fix: The refactor to use diamond syntax is correctly applied and should be used as a standard practice for similar instantiations throughout the codebase. It's a minor but impactful change that aligns with contemporary Java standards, promoting more maintainable and readable code. No additional modifications are needed for these specific lines. Developers should ensure that this practice is consistently applied in all applicable scenarios.
Code Suggestion:

+  static final Supplier<StatsCounter> CACHE_STATS_COUNTER = () -> new SimpleStatsCounter();

* because there isn't one yet.
*
* I would have guessed that a lambda would produce the same problem, but maybe it's safe because
* the lambda implementation is generated as a method in the _same class_ as the usage?
*/
static final Supplier<StatsCounter> CACHE_STATS_COUNTER = () -> new SimpleStatsCounter();
Comment on lines 231 to +243
Copy link

@BitoAgent BitoAgent Mar 25, 2024

Choose a reason for hiding this comment

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

Scalability Issue: Changing from an anonymous class to a lambda for the 'CACHE_STATS_COUNTER' initialization could potentially impact scalability, depending on how the JVM optimizes lambda expressions vs. anonymous classes. Lambdas in Java 8 are generally more efficient than anonymous classes, especially when they are stateless or effectively final, as they can be optimized better by the JVM.
Fix: The change to use a lambda expression is likely beneficial for scalability. However, it's important to benchmark and monitor the performance impact of this change in environments where the cache is under high load, to ensure that the theoretical scalability improvements are realized in practice.
Code Suggestion:

-      new Supplier<StatsCounter>() {
-        @Override
-        public StatsCounter get() {
-          return new SimpleStatsCounter();
-        }
-      };
+  /*
+   * We avoid using a method reference here for now: Inside Google, CacheBuilder is used from the
+   * implementation of a custom ClassLoader that is sometimes used as a system classloader. That's a
+   * problem because method-reference linking tries to look up the system classloader, and it fails
+   * because there isn't one yet.
+   *
+   * I would have guessed that a lambda would produce the same problem, but maybe it's safe because
+   * the lambda implementation is generated as a method in the _same class_ as the usage?
+   */
+  static final Supplier<StatsCounter> CACHE_STATS_COUNTER = () -> new SimpleStatsCounter();

Comment on lines +234 to +243
Copy link

@BitoAgent BitoAgent Mar 25, 2024

Choose a reason for hiding this comment

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

Optimization Issue: The replacement of an anonymous class with a lambda expression for CACHE_STATS_COUNTER improves readability and conciseness. This change also potentially enhances performance by reducing the amount of generated bytecode, as lambda expressions are more efficiently handled by the JVM compared to anonymous classes. However, the detailed comment explains a specific scenario where method references could introduce issues due to class loading order. This thoughtful consideration of context and potential issues is commendable, ensuring that the change does not inadvertently introduce bugs.
Fix: The change implemented is already optimal given the constraints described. No further action is required. The detailed comment provides valuable insight into why a lambda expression is used instead of a method reference, highlighting the importance of understanding the broader context in which code operates. Future modifications in this area should continue to take these considerations into account to maintain the integrity and reliability of the cache implementation.
Code Suggestion:

-      new Supplier<StatsCounter>() {
-        @Override
-        public StatsCounter get() {
-          return new SimpleStatsCounter();
-        }
-      };
+  /*
+   * We avoid using a method reference here for now: Inside Google, CacheBuilder is used from the
+   * implementation of a custom ClassLoader that is sometimes used as a system classloader. That's a
+   * problem because method-reference linking tries to look up the system classloader, and it fails
+   * because there isn't one yet.
+   *
+   * I would have guessed that a lambda would produce the same problem, but maybe it's safe because
+   * the lambda implementation is generated as a method in the _same class_ as the usage?
+   */
+  static final Supplier<StatsCounter> CACHE_STATS_COUNTER = () -> new SimpleStatsCounter();


enum NullListener implements RemovalListener<Object, Object> {
INSTANCE;
Expand Down Expand Up @@ -825,11 +828,11 @@ Ticker getTicker(boolean recordsTime) {
*
* <p><b>Warning:</b> after invoking this method, do not continue to use <i>this</i> cache builder
* reference; instead use the reference this method <i>returns</i>. At runtime, these point to the
* same instance, but only the returned reference has the correct generic type information so as
* to ensure type safety. For best results, use the standard method-chaining idiom illustrated in
* the class documentation above, configuring a builder and building your cache in a single
* statement. Failure to heed this advice can result in a {@link ClassCastException} being thrown
* by a cache operation at some <i>undefined</i> point in the future.
* same instance, but only the returned reference has the correct generic type information to
* ensure type safety. For best results, use the standard method-chaining idiom illustrated in the
* class documentation above, configuring a builder and building your cache in a single statement.
* Failure to heed this advice can result in a {@link ClassCastException} being thrown by a cache
* operation at some <i>undefined</i> point in the future.
*
* <p><b>Warning:</b> any exception thrown by {@code listener} will <i>not</i> be propagated to
* the {@code Cache} user, only logged via a {@link Logger}.
Expand Down
13 changes: 3 additions & 10 deletions android/guava/src/com/google/common/cache/CacheLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.errorprone.annotations.CheckReturnValue;
import java.io.Serializable;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.Executor;

/**
Expand Down Expand Up @@ -154,7 +153,7 @@ public static <K, V> CacheLoader<K, V> from(Function<K, V> function) {
*/
@CheckReturnValue
public static <V> CacheLoader<Object, V> from(Supplier<V> supplier) {
return new SupplierToCacheLoader<V>(supplier);
return new SupplierToCacheLoader<>(supplier);
}

private static final class FunctionToCacheLoader<K, V> extends CacheLoader<K, V>
Expand Down Expand Up @@ -195,15 +194,9 @@ public V load(K key) throws Exception {
}

@Override
public ListenableFuture<V> reload(final K key, final V oldValue) throws Exception {
public ListenableFuture<V> reload(final K key, final V oldValue) {
ListenableFutureTask<V> task =
ListenableFutureTask.create(
new Callable<V>() {
@Override
public V call() throws Exception {
return loader.reload(key, oldValue).get();
}
});
ListenableFutureTask.create(() -> loader.reload(key, oldValue).get());
executor.execute(task);
return task;
}
Comment on lines 195 to 202
Copy link

@BitoAgent BitoAgent Mar 25, 2024

Choose a reason for hiding this comment

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

Code Structure Issue: The method 'reload' has been refactored to use a lambda expression instead of an anonymous class, which simplifies the code and improves readability. Additionally, the 'throws Exception' declaration has been removed from the method signature, aligning with the lambda expression's inability to declare checked exceptions.
Fix: The refactor to use a lambda expression and the removal of the 'throws Exception' declaration from the 'reload' method are good improvements in code readability and conciseness. No further action is required.
Code Suggestion:

+      public ListenableFuture<V> reload(final K key, final V oldValue) {
+            ListenableFutureTask<V> task =
+                ListenableFutureTask.create(() -> loader.reload(key, oldValue).get());
+            executor.execute(task);
+            return task;
+          }

Expand Down
4 changes: 2 additions & 2 deletions android/guava/src/com/google/common/cache/CacheStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ public double missRate() {

/**
* Returns the total number of times that {@link Cache} lookup methods attempted to load new
* values. This includes both successful load operations, as well as those that threw exceptions.
* This is defined as {@code loadSuccessCount + loadExceptionCount}.
* values. This includes both successful load operations and those that threw exceptions. This is
* defined as {@code loadSuccessCount + loadExceptionCount}.
*
* <p><b>Note:</b> the values of the metrics are undefined in case of overflow (though it is
* guaranteed not to throw an exception). If you require specific handling, we recommend
Expand Down
85 changes: 35 additions & 50 deletions android/guava/src/com/google/common/cache/LocalCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Equivalence;
import com.google.common.base.Function;
import com.google.common.base.Stopwatch;
import com.google.common.base.Ticker;
import com.google.common.cache.AbstractCache.SimpleStatsCounter;
Expand Down Expand Up @@ -256,8 +255,8 @@ class LocalCache<K, V> extends AbstractMap<K, V> implements ConcurrentMap<K, V>
removalListener = builder.getRemovalListener();
removalNotificationQueue =
(removalListener == NullListener.INSTANCE)
? LocalCache.<RemovalNotification<K, V>>discardingQueue()
: new ConcurrentLinkedQueue<RemovalNotification<K, V>>();
? LocalCache.discardingQueue()
: new ConcurrentLinkedQueue<>();

ticker = builder.getTicker(recordsTime());
entryFactory = EntryFactory.getFactory(keyStrength, usesAccessEntries(), usesWriteEntries());
Expand Down Expand Up @@ -661,8 +660,8 @@ ValueReference<K, V> copyFor(
void notifyNewValue(@CheckForNull V newValue);

/**
* Returns true if a new value is currently loading, regardless of whether or not there is an
* existing value. It is assumed that the return value of this method is constant for any given
* Returns true if a new value is currently loading, regardless of whether there is an existing
* value. It is assumed that the return value of this method is constant for any given
* ValueReference instance.
*/
boolean isLoading();
Expand Down Expand Up @@ -1740,9 +1739,9 @@ Segment<K, V> createSegment(

/**
* Gets the value from an entry. Returns null if the entry is invalid, partially-collected,
* loading, or expired. Unlike {@link Segment#getLiveValue} this method does not attempt to
* cleanup stale entries. As such it should only be called outside of a segment context, such as
* during iteration.
* loading, or expired. Unlike {@link Segment#getLiveValue} this method does not attempt to clean
* up stale entries. As such it should only be called outside a segment context, such as during
* iteration.
*/
@CheckForNull
V getLiveValue(ReferenceEntry<K, V> entry, long now) {
Expand Down Expand Up @@ -1946,24 +1945,16 @@ static class Segment<K, V> extends ReentrantLock {
this.statsCounter = checkNotNull(statsCounter);
initTable(newEntryArray(initialCapacity));

keyReferenceQueue = map.usesKeyReferences() ? new ReferenceQueue<K>() : null;
keyReferenceQueue = map.usesKeyReferences() ? new ReferenceQueue<>() : null;

valueReferenceQueue = map.usesValueReferences() ? new ReferenceQueue<V>() : null;
valueReferenceQueue = map.usesValueReferences() ? new ReferenceQueue<>() : null;

recencyQueue =
map.usesAccessQueue()
? new ConcurrentLinkedQueue<ReferenceEntry<K, V>>()
: LocalCache.<ReferenceEntry<K, V>>discardingQueue();
map.usesAccessQueue() ? new ConcurrentLinkedQueue<>() : LocalCache.discardingQueue();

writeQueue =
map.usesWriteQueue()
? new WriteQueue<K, V>()
: LocalCache.<ReferenceEntry<K, V>>discardingQueue();
writeQueue = map.usesWriteQueue() ? new WriteQueue<>() : LocalCache.discardingQueue();

accessQueue =
map.usesAccessQueue()
? new AccessQueue<K, V>()
: LocalCache.<ReferenceEntry<K, V>>discardingQueue();
accessQueue = map.usesAccessQueue() ? new AccessQueue<>() : LocalCache.discardingQueue();
}

AtomicReferenceArray<ReferenceEntry<K, V>> newEntryArray(int size) {
Expand Down Expand Up @@ -2208,15 +2199,12 @@ ListenableFuture<V> loadAsync(
CacheLoader<? super K, V> loader) {
final ListenableFuture<V> loadingFuture = loadingValueReference.loadFuture(key, loader);
loadingFuture.addListener(
new Runnable() {
@Override
public void run() {
try {
getAndRecordStats(key, hash, loadingValueReference, loadingFuture);
} catch (Throwable t) {
logger.log(Level.WARNING, "Exception thrown during refresh", t);
loadingValueReference.setException(t);
}
() -> {
try {
getAndRecordStats(key, hash, loadingValueReference, loadingFuture);
} catch (Throwable t) {
logger.log(Level.WARNING, "Exception thrown during refresh", t);
loadingValueReference.setException(t);
}
},
directExecutor());
Expand Down Expand Up @@ -2482,7 +2470,7 @@ void drainRecencyQueue() {
// An entry may be in the recency queue despite it being removed from
// the map . This can occur when the entry was concurrently read while a
// writer is removing it from the segment or after a clear has removed
// all of the segment's entries.
// all the segment's entries.
if (accessQueue.contains(e)) {
accessQueue.add(e);
}
Expand Down Expand Up @@ -3266,7 +3254,7 @@ boolean reclaimValue(K key, int hash, ValueReference<K, V> valueReference) {
return false;
} finally {
unlock();
if (!isHeldByCurrentThread()) { // don't cleanup inside of put
if (!isHeldByCurrentThread()) { // don't clean up inside of put
postWriteCleanup();
}
}
Expand Down Expand Up @@ -3459,12 +3447,9 @@ public ListenableFuture<V> loadFuture(K key, CacheLoader<? super K, V> loader) {
// *before* returning newValue from the cache query.
return transform(
newValue,
new Function<V, V>() {
@Override
public V apply(V newValue) {
LoadingValueReference.this.set(newValue);
return newValue;
}
newResult -> {
LoadingValueReference.this.set(newResult);
return newResult;
},
directExecutor());
} catch (Throwable t) {
Expand Down Expand Up @@ -3807,19 +3792,19 @@ public boolean isEmpty() {
*/
long sum = 0L;
Segment<K, V>[] segments = this.segments;
for (int i = 0; i < segments.length; ++i) {
if (segments[i].count != 0) {
for (Segment<K, V> segment : segments) {
if (segment.count != 0) {
return false;
}
sum += segments[i].modCount;
sum += segment.modCount;
}

if (sum != 0L) { // recheck unless no modifications
for (int i = 0; i < segments.length; ++i) {
if (segments[i].count != 0) {
for (Segment<K, V> segment : segments) {
if (segment.count != 0) {
return false;
}
sum -= segments[i].modCount;
sum -= segment.modCount;
}
return sum == 0L;
}
Expand All @@ -3829,8 +3814,8 @@ public boolean isEmpty() {
long longSize() {
Segment<K, V>[] segments = this.segments;
long sum = 0;
for (int i = 0; i < segments.length; ++i) {
sum += Math.max(0, segments[i].count); // see https://github.com/google/guava/issues/2108
for (Segment<K, V> segment : segments) {
sum += Math.max(0, segment.count); // see https://github.com/google/guava/issues/2108
}
return sum;
}
Expand Down Expand Up @@ -4398,7 +4383,7 @@ public <E> E[] toArray(E[] a) {

private static <E> ArrayList<E> toArrayList(Collection<E> c) {
// Avoid calling ArrayList(Collection), which may call back into toArray.
ArrayList<E> result = new ArrayList<E>(c.size());
ArrayList<E> result = new ArrayList<>(c.size());
Iterators.addAll(result, c.iterator());
return result;
}
Expand Down Expand Up @@ -4654,7 +4639,7 @@ public ImmutableMap<K, V> getAll(Iterable<? extends K> keys) throws ExecutionExc
}

@Override
public final V apply(K key) {
public V apply(K key) {
return autoDelegate.apply(key);
}

Expand All @@ -4672,7 +4657,7 @@ static class LocalManualCache<K, V> implements Cache<K, V>, Serializable {
final LocalCache<K, V> localCache;

LocalManualCache(CacheBuilder<? super K, ? super V> builder) {
this(new LocalCache<K, V>(builder, null));
this(new LocalCache<>(builder, null));
}

private LocalManualCache(LocalCache<K, V> localCache) {
Expand Down Expand Up @@ -4770,7 +4755,7 @@ static class LocalLoadingCache<K, V> extends LocalManualCache<K, V>

LocalLoadingCache(
CacheBuilder<? super K, ? super V> builder, CacheLoader<? super K, V> loader) {
super(new LocalCache<K, V>(builder, checkNotNull(loader)));
super(new LocalCache<>(builder, checkNotNull(loader)));
}

// LoadingCache methods
Expand Down
29 changes: 16 additions & 13 deletions guava/src/com/google/common/cache/CacheBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
* <li>accumulation of cache access statistics
* </ul>
*
* <p>These features are all optional; caches can be created using all or none of them. By default
* <p>These features are all optional; caches can be created using all or none of them. By default,
* cache instances created by {@code CacheBuilder} will not perform any type of eviction.
*
* <p>Usage example:
Expand Down Expand Up @@ -230,13 +230,16 @@ public CacheStats snapshot() {
});
static final CacheStats EMPTY_STATS = new CacheStats(0, 0, 0, 0, 0, 0);

static final Supplier<StatsCounter> CACHE_STATS_COUNTER =
new Supplier<StatsCounter>() {
@Override
public StatsCounter get() {
return new SimpleStatsCounter();
}
};
/*
* We avoid using a method reference here for now: Inside Google, CacheBuilder is used from the
* implementation of a custom ClassLoader that is sometimes used as a system classloader. That's a
* problem because method-reference linking tries to look up the system classloader, and it fails
* because there isn't one yet.
*
* I would have guessed that a lambda would produce the same problem, but maybe it's safe because
* the lambda implementation is generated as a method in the _same class_ as the usage?
*/
static final Supplier<StatsCounter> CACHE_STATS_COUNTER = () -> new SimpleStatsCounter();

enum NullListener implements RemovalListener<Object, Object> {
INSTANCE;
Expand Down Expand Up @@ -926,11 +929,11 @@ Ticker getTicker(boolean recordsTime) {
*
* <p><b>Warning:</b> after invoking this method, do not continue to use <i>this</i> cache builder
* reference; instead use the reference this method <i>returns</i>. At runtime, these point to the
* same instance, but only the returned reference has the correct generic type information so as
* to ensure type safety. For best results, use the standard method-chaining idiom illustrated in
* the class documentation above, configuring a builder and building your cache in a single
* statement. Failure to heed this advice can result in a {@link ClassCastException} being thrown
* by a cache operation at some <i>undefined</i> point in the future.
* same instance, but only the returned reference has the correct generic type information to
* ensure type safety. For best results, use the standard method-chaining idiom illustrated in the
* class documentation above, configuring a builder and building your cache in a single statement.
* Failure to heed this advice can result in a {@link ClassCastException} being thrown by a cache
* operation at some <i>undefined</i> point in the future.
*
* <p><b>Warning:</b> any exception thrown by {@code listener} will <i>not</i> be propagated to
* the {@code Cache} user, only logged via a {@link Logger}.
Expand Down
13 changes: 3 additions & 10 deletions guava/src/com/google/common/cache/CacheLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.errorprone.annotations.CheckReturnValue;
import java.io.Serializable;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.Executor;

/**
Expand Down Expand Up @@ -153,7 +152,7 @@ public static <K, V> CacheLoader<K, V> from(Function<K, V> function) {
*/
@CheckReturnValue
public static <V> CacheLoader<Object, V> from(Supplier<V> supplier) {
return new SupplierToCacheLoader<V>(supplier);
return new SupplierToCacheLoader<>(supplier);
}

private static final class FunctionToCacheLoader<K, V> extends CacheLoader<K, V>
Expand Down Expand Up @@ -194,15 +193,9 @@ public V load(K key) throws Exception {
}

@Override
public ListenableFuture<V> reload(final K key, final V oldValue) throws Exception {
public ListenableFuture<V> reload(final K key, final V oldValue) {
ListenableFutureTask<V> task =
ListenableFutureTask.create(
new Callable<V>() {
@Override
public V call() throws Exception {
return loader.reload(key, oldValue).get();
}
});
ListenableFutureTask.create(() -> loader.reload(key, oldValue).get());
executor.execute(task);
return task;
}
Expand Down
4 changes: 2 additions & 2 deletions guava/src/com/google/common/cache/CacheStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ public double missRate() {

/**
* Returns the total number of times that {@link Cache} lookup methods attempted to load new
* values. This includes both successful load operations, as well as those that threw exceptions.
* This is defined as {@code loadSuccessCount + loadExceptionCount}.
* values. This includes both successful load operations and those that threw exceptions. This is
* defined as {@code loadSuccessCount + loadExceptionCount}.
*
* <p><b>Note:</b> the values of the metrics are undefined in case of overflow (though it is
* guaranteed not to throw an exception). If you require specific handling, we recommend
Expand Down
Loading