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

Records cannot be properly used in caches #3008

Open
JacobKeegan opened this issue Apr 11, 2022 · 7 comments
Open

Records cannot be properly used in caches #3008

JacobKeegan opened this issue Apr 11, 2022 · 7 comments
Labels

Comments

@JacobKeegan
Copy link

JacobKeegan commented Apr 11, 2022

If a cache is sized using MemoryUnit, and the cache key or value contains a record (which were added in Java 17), any attempt to put a value in the cache will fail. The core cause of this failure is this line in sun.misc.Unsafe:

if (declaringClass.isRecord()) {
     throw new UnsupportedOperationException("can't get field offset on a record class: " + f);
}

A simple project which shows the issue in practice is linked here.
This seems like a serious issue, because Ehcache gives no clear indication of why this failure happens, and records are natural to use as keys.

@chrisdennis
Copy link
Member

chrisdennis commented Apr 11, 2022

There are two ways round this right now:

  1. Not use byte based sizing.
  2. Configure an explicit size-of-engine to avoid the Unsafe size-of engine strategy. That's going to mean resorting to the reflection based sizing approach. Essentially duplicating the existing size-of-engine logic but with something like the following applied:
Index: ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngine.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngine.java b/ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngine.java
--- a/ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngine.java	(revision 8092a5b98f8fa27335377bf1884ac1d708028bd0)
+++ b/ehcache-impl/src/main/java/org/ehcache/impl/internal/sizeof/DefaultSizeOfEngine.java	(date 1649711593049)
@@ -25,6 +25,8 @@
 import org.ehcache.sizeof.SizeOfFilterSource;
 import org.ehcache.core.spi.store.Store;
 import org.ehcache.core.spi.store.heap.SizeOfEngine;
+import org.ehcache.sizeof.filters.CombinationSizeOfFilter;
+import org.ehcache.sizeof.impl.ReflectionSizeOf;
 
 /**
  * @author Abhilash
@@ -41,7 +43,7 @@
   public DefaultSizeOfEngine(long maxObjectGraphSize, long maxObjectSize) {
     this.maxObjectGraphSize = maxObjectGraphSize;
     this.maxObjectSize = maxObjectSize;
-    this.sizeOf = SizeOf.newInstance(new SizeOfFilterSource(true).getFilters());
+    this.sizeOf = new ReflectionSizeOf(new CombinationSizeOfFilter(new SizeOfFilterSource(true).getFilters()));
     this.onHeapKeyOffset = sizeOf.deepSizeOf(new CopiedOnHeapKey<>(new Object(), new IdentityCopier<>()));
     this.chmTreeBinOffset = sizeOf.deepSizeOf(ConcurrentHashMap.FAKE_TREE_BIN);
   }

Long term we're going to have to take a long hard look at the whole feature. The JDK is not moving in a direction such that any of this is getting easier. There's some obvious cleanup we can do that will make things better... but I'm wary of sunk-cost fallacies.

@linghengqian
Copy link

In the long run, should the byte-based cache be spun off as a separate dependency from Ehcache, the base Ehcache only provides a number-based cache and a range of applications?

@chrisdennis
Copy link
Member

chrisdennis commented Apr 13, 2022

In the long term (exactly how long is not clear...) we'll probably be dropping internal support for byte sized caches. I won't drop the ability to configure them... but there will be no built-in store provider that would handle those configurations. If/when this happens it would be associated with a bump of the Java support to 1.11 minimum, and a bunch of overhaul of the underlying code to take advantage of that.

JacobKeegan added a commit to nysenate/OpenLegislation that referenced this issue Apr 19, 2022
ehcache/ehcache3#3008
Cache sizes are determined when they are initialized, except for the 2 bill caches, which remain in app.properties.
@chrisdennis
Copy link
Member

This will require the productizing and merging of ehcache/sizeof#64.

@FeanorsCurse
Copy link

Hi, we just ran into this issue, with our DTOs being records :-/
Any chance this is fixed in the near future?

@linghengqian
Copy link

Hi, we just ran into this issue, with our DTOs being records :-/
Any chance this is fixed in the near future?

@FeanorsCurse
Copy link

Hi, thanks for the reply.
Not sure that helps in our case, config seems to be almost only xml. I managed a workaround by completely removing the offheap cache and only use in-memory.

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

No branches or pull requests

4 participants