You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cayenne.apache.org by "stariy95 (via GitHub)" <gi...@apache.org> on 2025/02/21 10:01:44 UTC
[PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
stariy95 opened a new pull request, #627:
URL: https://github.com/apache/cayenne/pull/627
Fix adds `ObjectStorePersistentWrapper` class that stores `Persistent` object with flatten paths in the same map, so it could be reclaimed by the GC.
This class implements `Persistent` interface for the maximum compatibility. And there are no changes to the public API (aside from the new class introduced).
There is still a minor risk of incompatibility as someone could rely on `ObjectStore` exact internal structure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "stariy95 (via GitHub)" <gi...@apache.org>.
stariy95 commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965481170
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -993,60 +1022,44 @@ public void arcDeleted(Object nodeId, Object targetNodeId, ArcId arcId) {
* @since 4.1
*/
boolean hasFlattenedPath(ObjectId objectId, String path) {
- if(trackedFlattenedPaths == null) {
- return false;
- }
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).containsKey(path);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.hasFlattenedPath(path);
}
/**
* @since 4.2
*/
public ObjectId getFlattenedId(ObjectId objectId, String path) {
- if(trackedFlattenedPaths == null) {
- return null;
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).get(path);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedId(path);
}
/**
* @since 4.2
*/
public Collection<ObjectId> getFlattenedIds(ObjectId objectId) {
- if(trackedFlattenedPaths == null) {
- return Collections.emptyList();
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).values();
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedIds();
}
/**
* @since 4.2.1
*/
public Map<String, ObjectId> getFlattenedPathIdMap(ObjectId objectId) {
- if(trackedFlattenedPaths == null) {
- return Collections.emptyMap();
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap());
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedPathIdMap();
}
/**
* Mark that flattened path for object has data row in DB.
* @since 4.1
*/
public void markFlattenedPath(ObjectId objectId, String path, ObjectId id) {
- if(trackedFlattenedPaths == null) {
- trackedFlattenedPaths = new ConcurrentHashMap<>();
- }
- trackedFlattenedPaths
- .computeIfAbsent(objectId, o -> new ConcurrentHashMap<>())
- .put(path, id);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.computeIfAbsent(objectId, objId -> {
+ System.out.println("Synthetic node for " + objId);
Review Comment:
No, should be deleted. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965667951
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
Btw with regards to streams and performance, it seems that the implementation of the values() method in ReferenceMap (super of SoftValueMap & WeakValueMap) doesn't help either.
https://github.com/apache/cayenne/blob/dc87bd62bdfbec0cf9a0a4ef14f429078d9d28c2/cayenne/src/main/java/org/apache/cayenne/util/ReferenceMap.java#L196-L211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965479185
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -993,60 +1022,44 @@ public void arcDeleted(Object nodeId, Object targetNodeId, ArcId arcId) {
* @since 4.1
*/
boolean hasFlattenedPath(ObjectId objectId, String path) {
- if(trackedFlattenedPaths == null) {
- return false;
- }
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).containsKey(path);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.hasFlattenedPath(path);
}
/**
* @since 4.2
*/
public ObjectId getFlattenedId(ObjectId objectId, String path) {
- if(trackedFlattenedPaths == null) {
- return null;
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).get(path);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedId(path);
}
/**
* @since 4.2
*/
public Collection<ObjectId> getFlattenedIds(ObjectId objectId) {
- if(trackedFlattenedPaths == null) {
- return Collections.emptyList();
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap()).values();
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedIds();
}
/**
* @since 4.2.1
*/
public Map<String, ObjectId> getFlattenedPathIdMap(ObjectId objectId) {
- if(trackedFlattenedPaths == null) {
- return Collections.emptyMap();
- }
-
- return trackedFlattenedPaths
- .getOrDefault(objectId, Collections.emptyMap());
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.get(objectId);
+ return wrapper.getFlattenedPathIdMap();
}
/**
* Mark that flattened path for object has data row in DB.
* @since 4.1
*/
public void markFlattenedPath(ObjectId objectId, String path, ObjectId id) {
- if(trackedFlattenedPaths == null) {
- trackedFlattenedPaths = new ConcurrentHashMap<>();
- }
- trackedFlattenedPaths
- .computeIfAbsent(objectId, o -> new ConcurrentHashMap<>())
- .put(path, id);
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) objectMap.computeIfAbsent(objectId, objId -> {
+ System.out.println("Synthetic node for " + objId);
Review Comment:
Does this console out stay?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "andrus (via GitHub)" <gi...@apache.org>.
andrus commented on PR #627:
URL: https://github.com/apache/cayenne/pull/627#issuecomment-2674507071
I am +1 on this approach in 4.2. I guess in 5.0 we should get rid of the Persistent contract of ObjectStorePersistentWrapper and make the internal ObjectStore map use explicit boundaries:
```
- protected Map<Object, Persistent> objectMap;
+ protected Map<ObjectId, SomeNewHolder> objectMap;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965550429
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
Okay :-) Speaking of performance though I'm concerned with all the casting happening in this PR, or am I mistaken ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "stariy95 (via GitHub)" <gi...@apache.org>.
stariy95 merged PR #627:
URL: https://github.com/apache/cayenne/pull/627
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "stariy95 (via GitHub)" <gi...@apache.org>.
stariy95 commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965555035
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
Static cast is a compile-time feature, so it should be ok. But additional wrapper would add some overhead unfortunately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "stariy95 (via GitHub)" <gi...@apache.org>.
stariy95 commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965533464
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
In any other place I would go with streams too in a such method. But in the Cayenne core we are trying to keep performance in mind, and leave streams out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1981020792
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -201,7 +205,7 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
}
}
- objectDiff = new ObjectDiff(object);
+ objectDiff = new ObjectDiff(persistentWrapper);
Review Comment:
Was there a particular reason that ObjectDiff was modified ?
It doesn't seem to require any functionality from ObjectStorePersistentWrapper ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965515190
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
Alternative instead of using AtomicInteger maybe ?
```java
return (int) objectMap.values().stream()
.map( obj -> (ObjectStorePersistentWrapper) obj )
.filter( wrap -> wrap.hasObject() )
.count();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "stariy95 (via GitHub)" <gi...@apache.org>.
stariy95 commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1981049296
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -201,7 +205,7 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
}
}
- objectDiff = new ObjectDiff(object);
+ objectDiff = new ObjectDiff(persistentWrapper);
Review Comment:
Yes, though it's not obviuos. Diff must hold a reference to the ObjectStore internal state object, as otherwise it may be deleted by the GC from the Weak/Soft reference map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAY-2876 Memory leak in the ObjectStore [cayenne]
Posted by "Jugen (via GitHub)" <gi...@apache.org>.
Jugen commented on code in PR #627:
URL: https://github.com/apache/cayenne/pull/627#discussion_r1965667951
##########
cayenne-server/src/main/java/org/apache/cayenne/access/ObjectStore.java:
##########
@@ -213,13 +217,28 @@ synchronized ObjectDiff registerDiff(Object nodeId, NodeDiff diff) {
return objectDiff;
}
+ private Persistent getUnwrapped(Object nodeId) {
+ Persistent persistent = objectMap.get(nodeId);
+ if(persistent == null) {
+ return null;
+ }
+ return ((ObjectStorePersistentWrapper) persistent).dataObject();
+ }
+
/**
* Returns a number of objects currently registered with this ObjectStore.
*
* @since 1.2
*/
public int registeredObjectsCount() {
- return objectMap.size();
+ AtomicInteger counter = new AtomicInteger();
+ objectMap.forEach((id, obj) -> {
+ ObjectStorePersistentWrapper wrapper = (ObjectStorePersistentWrapper) obj;
+ if(wrapper.hasObject()){
+ counter.incrementAndGet();
+ }
+ });
+ return counter.get();
Review Comment:
Btw with regards to streams and performance, it seems that the implementation of the values() method in ReferenceMap (super of SoftValueMap & WeakValueMap) doesn't help either.
https://github.com/apache/cayenne/blob/dc87bd62bdfbec0cf9a0a4ef14f429078d9d28c2/cayenne/src/main/java/org/apache/cayenne/util/ReferenceMap.java#L196-L211
Good to know :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cayenne.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org