Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.getUnchecked(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final LoadingCache<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new LoadingCache<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.getUnchecked(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final LoadingCache<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new LoadingCache<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.getUnchecked(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final LoadingCache<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new LoadingCache<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );
added 11 characters in body
Source Link
maaartinus
  • 13.7k
  • 1
  • 35
  • 74

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.getgetUnchecked(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final CacheLoader<ResourceKeyLoadingCache<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new CacheLoader<ResourceKeyLoadingCache<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.get(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final CacheLoader<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new CacheLoader<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.getUnchecked(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final LoadingCache<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new LoadingCache<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );
Source Link
maaartinus
  • 13.7k
  • 1
  • 35
  • 74

Something between a comment on rolfl's review and an own CR.

DCL

DCL is broken, but can be easily helped using volatile. Without it, it may happen that an object gets loaded, but other threads see its non-initialized state, which is too bad.

This wouldn't happen for objects with final variables fully initialized in the constructor, but imposing such a constraint on the user is cruel and the punishment in form of inexplicable random bugs even more so.

###AutoCloseable

I'd prefer Closeable to AutoCloseable as it guarantees idempotency and that's how it behaves. But I agree that leaving it out is better.

Caching / ReferenceQueue

I guess that the part concerning caching is fine. A ReferenceQueue would load things which may not be needed anymore, that's more a threaded memory hog than a cache. I actually agree with the design in this respect.

No, I don't need a soft reference.

You most probably do, unless it's OK to lose entries on each GC, and then you could use a WeakReference. But the standard for caching is SoftReference.

Note that there were bugs when the JVM threw OOM before cleaning all soft reference. That's why they got rather unpopular and some whatever-stupid-criterion eviction is often preferred.

Alternative

Your single element cache could be implemented via a normal (Guava) cache.

@RequiredArgsConstructor
@Getter
class ResourceKey<T> {
 public T get() {
 return CACHE.get(this);
 }
 private final ByteSource source;
 private final ResourceLoader<T> loader;
}
private static final CacheLoader<ResourceKey, T> CACHE =
 CacheBuilder.newBuilder().softValues().build(
 new CacheLoader<ResourceKey, T>() {
 public T load(ResourceKey key) {...}
 }
 );
lang-java

AltStyle によって変換されたページ (->オリジナル) /