Issue Details (XML | Word | Printable)

Key: NUCCORE-1053
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Unassigned
Reporter: Cyril Bouteille
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
DataNucleus Core

Unexpected L2 cache look-ups for JDO with cacheable=false

Created: 18/Jun/13 01:25 AM   Updated: 27/Jun/13 04:39 PM   Resolved: 22/Jun/13 06:14 AM
Component/s: Cache
Affects Version/s: 3.2.4
Fix Version/s: 3.2.5

File Attachments: 1. File datanucleus-l2-issue.tgz (4 kB)
2. Java Source File ExecutionContextImpl.java (203 kB)

Environment:
JDK 1.7
RHEL

Datastore: MySQL
Severity: Test/Acceptance


 Description  « Hide
We're experiencing bottlenecks in our performance testing which we narrowed down the lock wait in xmemcached plugin.
We tried switching to spymemcached, but it didn't make a significant difference.
Upon further investigation we uncovered an unexpectedly large amount of L2 cache activity, most of which are for JDO classes which are marked as cacheable=false in the metadata.
It looks like the cache puts are guarded by cacheable=false, but not the gets.
If this is not intended or otherwise necessary, could the code be optimized to assume a cache miss if the class is not cacheable?
Thanks.
We made a test case we will be uploading.

Sort Order: Ascending order - Click to sort in descending order
Cyril Bouteille added a comment - 18/Jun/13 01:26 AM
Test case showing L2 GET for JDO class cacheable=false

(org.datanucleus.test.Main.main()) DEBUG [DataNucleus.Cache] - Object with id "org.datanucleus.test.order.PayOrder:1" not found in Level 2 cache

Andy Jefferson added a comment - 18/Jun/13 07:16 PM
Yes, all it does currently is stop uncacheable things getting in the L2 cache. You could easily enough provide a patch to add checks on the get(s), attach it to this issue. Suggest that your patch gets the class name from the object id for OID/SingleFieldIdentity cases and checks that class name (nucCtx.isClassCacheable).

Cyril Bouteille added a comment - 20/Jun/13 03:24 AM
We'd just need to make the lookup conditional on that basis indeed, but not being familiar with the internal APIs of DN it's not obvious to us... :)
Something like below in datanucleus-core-3.2.4-sources/org/datanucleus/ExecutionContextImpl.java?

5233a5234,5249
> // verify that class is cacheable
> String pClassName = null;
> if (id instanceof OID) {
> pClassName = ((OID)id).getPcClass();
> }
> else if (getApiAdapter().isSingleFieldIdentity(id)) {
> pClassName = getApiAdapter().getTargetClassNameForSingleFieldIdentity(id);
> }
> else {
> throw new NucleusException("getObjectFromLevel2Cache encountered unknown Object id=" + id
> + " that is neither datastore-identity/SingleFieldIdentity");
> }
> AbstractClassMetaData pClassMetaData = getMetaDataManager()
> .getMetaDataForClass(pClassName, clr);
>
> if (nucCtx.isClassCacheable(pClassMetaData)) {
5270a5287
> }


Cyril Bouteille added a comment - 20/Jun/13 07:18 PM
Attaching the patched file if preferable over the diff.

Andy Jefferson added a comment - 20/Jun/13 07:56 PM
1. patch is the preferred format for anything, as per the docs.
2. your proposal means that someone who wants to have compound identity or composite identity will fail on anything. If an id is not OID or SingleFieldIdentity then you should ASSUME it is cacheable. This patch is (should be) an optimisation for the OID and SingleFieldIdentity cases to not go near the cache.
3. When you have a patch, please comment on whether that works for your case(s).

Cyril Bouteille added a comment - 21/Jun/13 12:57 AM
2. we reproduced the issue for composite object id classes but feel there are scenarios where developers may still want those to be non-cacheable (we actually have some already), so we enhanced the logic based on AbstractStoreManager.getClassNameForObjectID, but we're not able to leverage it directly as apparently the RDBMS subclass is calling the method we're patching thereby causing an infinite loop :)

5233a5234,5262
> String pClassName = null;
> if (id instanceof SCOID)
> {
> pClassName = ((SCOID)id).getSCOClass();
> }
> else if (id instanceof OID)
> {
> pClassName = ((OID)id).getPcClass();
> }
> else if (getApiAdapter().isSingleFieldIdentity(id))
> {
> pClassName = getApiAdapter().getTargetClassNameForSingleFieldIdentity(id);
> }
>
> AbstractClassMetaData pClassMetaData = null;
> if (pClassName != null) {
> pClassMetaData = getMetaDataManager().getMetaDataForClass(pClassName, clr);
> }
> else
> { // Application identity with user PK class, so find all using this PK
> Collection<AbstractClassMetaData> cmds = getMetaDataManager()
> .getClassMetaDataWithApplicationId(id.getClass().getName());
> if (cmds != null && !cmds.isEmpty())
> { // get first random one
> pClassMetaData = cmds.iterator().next();
> }
> }
>
> if (pClassMetaData == null || nucCtx.isClassCacheable(pClassMetaData)) {
5270a5300
> }

3. no issue found with this patch on our side so far but let us know if the unit test suite uncovers anything

Let me know your thoughts.
Thanks!

Andy Jefferson added a comment - 21/Jun/13 10:08 AM
I added a variation on your code to SVN trunk, NucleusContext.isClassWithIdentityCacheable and a call to it from ExecutionContext. Causes no problems with DN tests, JDO TCK or JPA TCK. Report back if it works on your cases.

One potential problem is the composite PK case if the first of the classes with the PK class is not cacheable yet others are, but then that's a minority use-case.

Cyril Bouteille added a comment - 22/Jun/13 02:25 AM
Great, thanks!
I don't have the whole env to build trunk, but I backported http://sourceforge.net/p/datanucleus/code/17616/ to 3.2.4, patched datanucleus-core-3.2.4.jar and verified it works as well in our app env.
Please let us know which version you plan to target this fix for.
Thank you
Cyril

Andy Jefferson added a comment - 22/Jun/13 06:14 AM
I don't see a need to "build" anything, that's what nightly builds are and always have been (under Download on the website). This will be 3.2.5 of "datanucleus-core", which will be part of AccessPlatform 3.2.4 and 3.3.0-m2