[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473416
]
Abe White commented on OPENJPA-141:
---
The two-way check in FetchConfigurationImpl was overlooked. Thank you. But,
that brings up a new question... Do we do the two-way check in this new
utility method (even though BrokerImpl didn't require this in the past)? Or,
is the one-way check sufficient for FetchConfigurationImpl's usage?
It should be a one-way check and code that needs to check both directions
should invoke it twice with the arguments swapped the second time. Simple.
There have been several viewpoints on the use of these reference types and
what the impact would be. To be honest, at this point, all that I am looking
for is the ability to cache these assignable types. Whether it's
redployment-friendly or memory-friendly, I don't really care at this point.
I don't care either, so long as it's one of the two reference type combinations
I outlined that actually make sense. We can't just pick reference types
randomly.
As to whether we should use a static cache vs. a Configuration instance
cache... doesn't matter much to me. The static cache keeps the API simpler.
Definitely beneficial over no caching of the TM whatsoever. Sorry for the
confusion.
So the question now is whether it is beneficial over the use of an instance
variable. In normal usage, there's no reason it should be. And so we should
use an instance variable. You could code an entire app using static maps in
place of all instance variables if you really wanted to, so yes, in that sense
they're equivalent, but there are good reasons not to. Same goes for this.
And finally, I'm going to harp on something that is no longer even relevant,
because I'm sure it will come up again in API design moving forward:
==
5. Even if ImplHelper.isAssignable retains its map parameter (and per #1
above I question why it should), it should just be a Map; I don't see why
you'd have the method require a ConcurrentMap.
I did this way to be thread safe. If I only used a Map parameter, then the
caller would have to ensure that any updates to the Cache are thread safe.
The caller is giving you the Map in your scheme. It's up to him whether the
Map he's giving you is used concurrently or not. The helper method itself has
no threading issues at all, and only requires a Map. But I agree that if we
move to a single cache in ImplHelper it's a moot point.
That's definitely one way around it. I prefer to enforce the requirement via
the signature of the contract. ===
This is a stateless static helper method. You're not enforcing anything,
because the method itself has no threading concerns whatsoever. You're
*imposing* a requirement to use a certain kind of Map on a method that would
function perfectly well with anything that implements the Map interface. That
is not something a helper method should do. What if I want to use the method
from single threaded code? What if I want to use it from a method that's
already synchronized? Or what if I want to pass it a synchronized map instead
of a concurrent one? Why should a helper method that knows nothing about who
is calling it and has no threading concerns itself be forcing any concurrency
strategy on me, much less a particular one? By this reasoning, every public
method in the entire codebase should either be synchronized or should require
thread safe arguments, because someone who calls the method *might* be doing so
from multi-threaded code that is not already synchronized.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473527
]
Craig Russell commented on OPENJPA-141:
---
Just posted another version of the performance patch for openjpa-141. I
believe I have honored the comments as posted to this Issue. Please take a
look and let's see if we can put this one to bed...
Looks good.
I have more performance-related items to pursue... :-)
I was hoping you did.
Thanks!
Thanks for your patience.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt, openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473537
]
Patrick Linskey commented on OPENJPA-141:
-
That's because I ran up against the other OpenJPA convention of limiting the
line length to 80 characters.
Ah, yes... the clash of the code standards.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt, openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
-_typeHash = base.hashCode();
+Integer typeHashInt = (Integer) _typeCache.get(type);
+if (typeHashInt == null) {
+
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473172
]
Abe White commented on OPENJPA-141:
---
1. Why not keep a single assignable types map in ImplHelper?
2. I thought we had decided on the assignable types map having hard keys and
soft values. Using soft keys and hard values is odd to say the least. First,
as I mentioned in a previous note, using soft Class keys is pointless. Once a
Class is eligible for GC there's no point in keeping it in cache, so weak is
better. Second, using hard values means that other than adapting to class
redeploys, this is basically a hard cache, because the only time entries are
removed is when a Class disappears, and that only happens on redeploy. It's
not necessarily bad to make this a hard cache, but it should be discussed.
3. Why keep dedicated isAssignable methods in BrokerImpl and
FetchConfigurationImpl if all they do is delegate to ImplHelper? Why not call
ImplHelper directly?
4. Why are you using a static JNDI location - TM cache in
JNDITransactionManager rather than just caching the TM in an instance variable?
The only time that would help performance is if you're creating a bunch of
BrokerFactories all using the same TM location. Most applications will only
use a single BrokerFactory. If your benchmarks is constantly creating
BrokerFactories, I'd question the validity of the benchmark.
5. Even if ImplHelper.isAssignable retains its map parameter (and per #1 above
I question why it should), it should just be a Map; I don't see why you'd have
the method require a ConcurrentMap.
6. #2 above applies also to the Class-base hash map in OpenJPAId.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473228
]
Kevin Sutter commented on OPENJPA-141:
--
Good thing I posted the patch... ;-)
1. Why not keep a single assignable types map in ImplHelper?
I actually thought about doing that, but then I had concerns about the size of
the Cache. So, I decided to isolate the changes (and the caches) to the
problems at hand. If you don't see a problem with this, I can change to a
single assignable types map.
2. I thought we had decided on the assignable types map having hard keys and
soft values. Using soft keys and hard values is odd to say the least. First,
as I mentioned in a previous note, using soft Class keys is pointless. Once a
Class is eligible for GC there's no point in keeping it in cache, so weak is
better. Second, using hard values means that other than adapting to class
redeploys, this is basically a hard cache, because the only time entries are
removed is when a Class disappears, and that only happens on redeploy. It's
not necessarily bad to make this a hard cache, but it should be discussed.
Hmmm.. If I understand what you are saying, it really doesn't matter whether
we use hard, weak, or soft keys, since the resulting cache will be hard no
matter what -- since we're using Class objects in the cache. And, using weak
keys actually sounds better due to the class redeploy scenario. I can
understand your hesitance with soft keys, but from your arguments, it sounds
like we should go with weak keys and hard values.
3. Why keep dedicated isAssignable methods in BrokerImpl and
FetchConfigurationImpl if all they do is delegate to ImplHelper? Why not call
ImplHelper directly?
Certainly could. Cleans up the code a bit.
4. Why are you using a static JNDI location - TM cache in
JNDITransactionManager rather than just caching the TM in an instance
variable? The only time that would help performance is if you're creating a
bunch of BrokerFactories all using the same TM location. Most applications
will only use a single BrokerFactory. If your benchmarks is constantly
creating BrokerFactories, I'd question the validity of the benchmark.
It's probably six of one, half a dozen of the other. I can make the change to
use an instance variable.
The benchmark is a set of primitives based on the SpecJApp application using
the SunOne Application Server. The profiling data from this set of tests
indicate that caching of the JNDI lookup is beneficial. Maybe this change only
helps with this particular Application Server, but I'm trying to isolate our
OpenJPA implementation from the RI (SunOne).
5. Even if ImplHelper.isAssignable retains its map parameter (and per #1 above
I question why it should), it should just be a Map; I don't see why you'd have
the method require a ConcurrentMap.
I did this way to be thread safe. If I only used a Map parameter, then the
caller would have to ensure that any updates to the Cache are thread safe.
Instead of putting that :artificial requirement on the caller, why not just
use the ConcurrentMap type to ensure the safety? Of course, this point is
moot, if you are okay with going with a single Cache.
6. #2 above applies also to the Class-base hash map in OpenJPAId.
Yep, all the same issue.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473234
]
Abe White commented on OPENJPA-141:
---
Craig, good catch. I didn't even look at the actual assignable method code...
I was saving that for when we had settled on a caching strategy.
1. Why not keep a single assignable types map in ImplHelper?
I actually thought about doing that, but then I had concerns about the size
of the Cache.
How are two static maps going to end up being smaller overall than one combined
static map?
Hmmm.. If I understand what you are saying, it really doesn't matter whether
we use hard, weak, or soft keys, since the resulting cache will be hard no
matter what -- since we're using Class objects in the cache.
No, it does matter. And the type of value references we use matters way more.
If we want a hard cache that drops entries for classes that are redeployed,
then we should be using weak keys and hard values. If we want a memory
sensitive cache, then we should be using hard keys and soft values. I'm not
sure where the disconnect is coming from with these reference types.
The benchmark is a set of primitives based on the SpecJApp application using
the SunOne Application Server. The profiling data from this set of tests
indicate that caching of the JNDI lookup is beneficial.
Beneficial over the suggested use of an instance variable? Or beneficial over
no caching of the TM whatsoever? There's a big difference.
5. Even if ImplHelper.isAssignable retains its map parameter (and per #1
above I question why it should), it should just be a Map; I don't see why
you'd have the method require a ConcurrentMap.
I did this way to be thread safe. If I only used a Map parameter, then the
caller would have to ensure that any updates to the Cache are thread safe.
The caller is giving you the Map in your scheme. It's up to him whether the
Map he's giving you is used concurrently or not. The helper method itself has
no threading issues at all, and only requires a Map. But I agree that if we
move to a single cache in ImplHelper it's a moot point.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473243
]
Kevin Sutter commented on OPENJPA-141:
--
Yes, good catch, Craig. The original location that I wanted to resolve the
isAssignableFrom() overhead was in BrokerImpl and that was just a one-way
check. The two-way check in FetchConfigurationImpl was overlooked. Thank you.
But, that brings up a new question... Do we do the two-way check in this new
utility method (even though BrokerImpl didn't require this in the past)? Or,
is the one-way check sufficient for FetchConfigurationImpl's usage? Any
historical perspective?
1. Why not keep a single assignable types map in ImplHelper?
I actually thought about doing that, but then I had concerns about the size
of the Cache.
How are two static maps going to end up being smaller overall than one
combined static map?
Never mind. Since we're dealing with (hopefully) unique Class keys, a single
map will suffice.
Hmmm.. If I understand what you are saying, it really doesn't matter whether
we use hard, weak, or soft keys, since the resulting cache will be hard no
matter what -- since we're using Class objects in the cache.
No, it does matter. And the type of value references we use matters way more.
If we want a hard cache that drops entries for classes that are redeployed,
then we should be using weak keys and hard values. If we want a memory
sensitive cache, then we should be using hard keys and soft values. I'm not
sure where the disconnect is coming from with these reference types.
There have been several viewpoints on the use of these reference types and what
the impact would be. To be honest, at this point, all that I am looking for is
the ability to cache these assignable types. Whether it's redployment-friendly
or memory-friendly, I don't really care at this point. We can worry about that
later. If you have a preference for this first iteration, let me know. Thanks.
The benchmark is a set of primitives based on the SpecJApp application using
the SunOne Application Server. The profiling data from this set of tests
indicate that caching of the JNDI lookup is beneficial.
Beneficial over the suggested use of an instance variable? Or beneficial over
no caching of the TM whatsoever? There's a big difference.
Definitely beneficial over no caching of the TM whatsoever. Sorry for the
confusion.
5. Even if ImplHelper.isAssignable retains its map parameter (and per #1
above I question why it should), it should just be a Map; I don't see why
you'd have the method require a ConcurrentMap.
I did this way to be thread safe. If I only used a Map parameter, then the
caller would have to ensure that any updates to the Cache are thread safe.
The caller is giving you the Map in your scheme. It's up to him whether the
Map he's giving you is used concurrently or not. The helper method itself has
no threading issues at all, and only requires a Map. But I agree that if we
move to a single cache in ImplHelper it's a moot point.
That's definitely one way around it. I prefer to enforce the requirement via
the signature of the contract. Changing to a single map anyway...
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473254
]
Patrick Linskey commented on OPENJPA-141:
-
If we want a hard cache that drops entries for classes that are redeployed,
then
we should be using weak keys and hard values. If we want a memory sensitive
cache, then we should be using hard keys and soft values.
I vaguely prefer a hard cache that drops entries for classes that are
redeployed, since I care more about speed than memory footprint.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base =
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473285
]
Craig Russell commented on OPENJPA-141:
---
If we want a hard cache that drops entries for classes that are
redeployed, then
we should be using weak keys and hard values. If we want a memory
sensitive
cache, then we should be using hard keys and soft values.
I vaguely prefer a hard cache that drops entries for classes that are
redeployed, since I care more about speed than memory footprint.
This is also affected by whether we use a giant cache (including all the EMFs)
or one cache per EMF. I prefer one cache per EMF because it simplifies the
cache entry life cycle. During EMF close, the cache can be explicitly cleared,
which allows the garbage collector to be more efficient since it doesn't have
to scan all the entries in the cache.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Attachments: openjpa-141.txt
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id.
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472758
]
Abe White commented on OPENJPA-141:
---
Craig:
There doesn't appear to be any hashCode/equals performance issues. As Kevin
pointed out earlier in the thread (and I should have realized) he's only
caching Class-hash in OpenJPAId to prevent having to traverse to the base
class... apparently Class.getSuperclass() is slow. It could just as well be a
cache of Class-base Class, but given the restriction Kevin pointed out that
ConcurrentReferenceHashMaps need at least one ref type to be hard, that'd
result in a cache that would maintain refs to classes that should be dropped
due to redeployment.
Kevin:
It's unfortunate that ConcurrentReferenceHashMaps need at least one ref type to
be hard. It would have been nice to use weak Class keys and soft values for
most of the caches. As is, I guess hard keys will do... we'll rely on the
value getting GC'd to expire entries for classes that disappear due to
redeployment. Note that the inner maps in your map-of-maps scheme of
assignables might as well just be a normal ConcurrentHashMaps too -- the outer
map already will let inner maps drop when memory is low. I also agree with
craig about mapping to Boolean.TRUE/FALSE instead of new Object(). And I'd
still like to see the assignable logic moved to ImplHelper or some new common
helper class.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a)
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472790
]
Kevin Sutter commented on OPENJPA-141:
--
Sounds good, Abe. Thanks for the comments, Abe and Craig. Here's the plan for
the changes (for this Issue)...
o Fix up the TM caching
o Change to using Hard/Soft reference types as required
o Store true/false in the inner map
o Make isAssignable common code
o Use ConcurrentHashMap for the inner class
That should cover it for now.
Thanks,
Kevin
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
-_typeHash =
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472795
]
Craig Russell commented on OPENJPA-141:
---
Nice work, Kevin and Abe.
Just a reminder. Please attach an svn patch to this JIRA when it's ready. I'd
like to be able to see the patch right along side this discussion here. And
when the patch is committed, please put OPENJPA-141 into the commit comments so
svn will back link to this page. Thanks.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
-_typeHash = base.hashCode();
+Integer typeHashInt =
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472813
]
Patrick Linskey commented on OPENJPA-141:
-
Please attach an svn patch to this JIRA when it's ready. I'd like to be able
to see the
patch right along side this discussion here. And when the patch is committed,
please put OPENJPA-141 into the commit comments so svn will back link to this
page.
Actually, if you put OPENJPA-141 in the comments, JIRA will automatically
create a link to the commit in svn, which allows you to see a patch. IMO, if
the issue number goes in the commit message, then we needn't bother with a
patch.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() !=
Re: [jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
Exactly.
On 2/13/07, Patrick Linskey (JIRA) [EMAIL PROTECTED] wrote:
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472813]
Patrick Linskey commented on OPENJPA-141:
-
Please attach an svn patch to this JIRA when it's ready. I'd like to be
able to see the
patch right along side this discussion here. And when the patch is
committed,
please put OPENJPA-141 into the commit comments so svn will back link to
this page.
Actually, if you put OPENJPA-141 in the comments, JIRA will automatically
create a link to the commit in svn, which allows you to see a patch. IMO, if
the issue number goes in the commit message, then we needn't bother with a
patch.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be
working with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472829
]
Craig Russell commented on OPENJPA-141:
---
Given the number of iterations already documented on this issue, wouldn't it be
better to attach the real patch here for review?
In other words, this issue might be a case of review then commit instead of
our normal commit then review approach.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
-_typeHash = base.hashCode();
+Integer typeHashInt = (Integer) _typeCache.get(type);
+if
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472840
]
Kevin Sutter commented on OPENJPA-141:
--
Personally, I think I provide sufficient due diligence on the Issues that I own
to stick with the normal commit then review approach. There are many, many
changes that get incorporated into OpenJPA without even a JIRA Issue
discussion. So, until I totally screw up, I'm going along with Patrick's
comment and will do my normal commit with the JIRA Issue in the comment field.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so that it doesn't have to be generated each time.
+ */
public int hashCode() {
if (_typeHash == 0) {
-Class base = type;
-while (base.getSuperclass() != null
- base.getSuperclass() != Object.class)
-base = base.getSuperclass();
-
Re: [jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
Hi Kevin, On Feb 13, 2007, at 11:25 AM, Kevin Sutter (JIRA) wrote: Kevin Sutter commented on OPENJPA-141: -- Personally, I think I provide sufficient due diligence on the Issues that I own to stick with the normal commit then review approach. There are many, many changes that get incorporated into OpenJPA without even a JIRA Issue discussion. So, until I totally screw up, I don't expect that any committer is going to totally screw up, and I hope you don't take my comments as accusing you of such. But this issue seems to be full of interesting nuances that might better be discussed before the fact. This issue is already the result of a commit with serious issues raised against it after the fact. So I'm a bit perplexed at the emotional reaction to my request to separate this steer from the herd. I'm going along with Patrick's comment and will do my normal commit with the JIRA Issue in the comment field. Certainly this is your choice. Just one final comment. The Apache Way calls for thorough and public discussion on contentious issues, and this is complicated substantially by having the appearance of shadow implementers, as reflected in your comments in the original JIRA: I will be working with Abe and my performance team to work through these issues. Craig More performance improvements (in response to changes for OPENJPA-138) - - Key: OPENJPA-141 URL: https://issues.apache.org/jira/browse/ OPENJPA-141 Craig Russell Architect, Sun Java Enterprise System http://java.sun.com/products/jdo 408 276-5638 mailto:[EMAIL PROTECTED] P.S. A good JDO? O, Gasp! smime.p7s Description: S/MIME cryptographic signature
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472440
]
Kevin Sutter commented on OPENJPA-141:
--
Re: Caching the TransactionManager in a static...
Although we could cache the TM in an instance variable or even do a hashmap
with the name as the key, I'm wondering what the scenario is where we could
have multiple TMs at different JNDI locations. I've looked at the code and it
seems that we have a single TM per runtime instance and we have one of those
per configuration. We attempt to find or create an appropriate runtime
instance which in turn creates an appropriate TM. Where or how can we run into
the multiple TM problem?
Re: Caching of the assignable from/to types
Yeah, the HARD/WEAK reference types probably weren't the right choices. For
the outer map, we could change to SOFT/SOFT references, and the inner map we
could change to SOFT/WEAK references (since we don't really care about the
values in the inner map). We'll do some experimenting with this to determine
the proper configuration.
Moving this common isAssignable code to ImplHelper or something similar is a
good suggestion. I had the same thoughts, but ran out of time over the
weekend...
Re: Use a different JVM
We are actually doing these performance runs using the Sun JDK. We are also
verifying the results with the IBM JDK, but the Sun JDK is the one used for the
primary performance runs and analysis.
Re: Hashcode performance
It's actually not the calls to Class.hashCode() that take up so much time, it's
the calls to Class.getSuperClass() that we're trying to avoid. I'm not
familiar with the System.identityHashCode() method. Maybe this method does the
proper getSuperClass processing more efficiently? Not sure.
Also, same comment about the HARD/WEAK references. I'll probably end up
changing that to SOFT/SOFT.
Re: Class.getClassLoader performance
Unfortunately, this call is expensive. Although in theory, this invocation
should be cheap since each Class needs a reference to its ClassLoader, the
caching of the ClassLoader is beneficial. Here again, this is with the Sun JDK
as our primary JVM.
Re: Overall
As stated previously, we are using the Sun JDK for all of these performance
benchmarks. We also verify the results using the IBM JDK. (We've even done
some comparisons with JRockit.) I've asked our performance team to generalize
our results for consumption by the OpenJPA community (we're not ready to go
public with any specific numbers). I should be able to post something shortly.
Thanks,
Kevin
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472455
]
Abe White commented on OPENJPA-141:
---
1. Each BrokerFactory has a ManagedRuntime. You can have multiple
BrokerFactories, each of which is supposed to be completely independent.
Therefore you can have multiple ManagedRuntimes, each of which is supposed to
be completely independent. Caching the TM in a static in JNDIManagedRuntime
breaks that.
2. I still don't understand how the caches were working at all with the weak
refs, unless GC just wasn't kicking in very often. Any info on this?
As to the proposed changes: there's no point in caching on Class keys with soft
refs. As soon as a Class is no longer referenced anywhere, there's no point in
keeping it around. So all Class keys should be weak refs. The corresponding
values can be soft.
3. System.identityHashCode isn't going to help with superclasses... I didn't
realize that that was the slow part.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472478
]
Craig Russell commented on OPENJPA-141:
---
Abe opined:
1. Each BrokerFactory has a ManagedRuntime. You can have multiple
BrokerFactories, each of which is supposed to be completely independent.
Therefore you can have multiple ManagedRuntimes, each of which is supposed to
be completely independent. Caching the TM in a static in JNDIManagedRuntime
breaks that.
Craig thinks: +1. There is no reason to optimize the number of lookups of the
ManagedRuntime, since it's only done once per EMF creation. I agree that making
the reference static goes too far.
2. I still don't understand how the caches were working at all with the weak
refs, unless GC just wasn't kicking in very often. Any info on this?
Craig thinks: Weak references are supposed to be cleaned up if the referenced
instance is not otherwise referenced. What would cause the referred classes to
be garbage collected immediately? I don't quite understand the issue here.
But this might beg the real issue, which is what to use as the key for the Map
if you want to effectively use the Class as the key but the hashCode and equals
methods are just too slow. It might be well to look more closely at
IdentityHashMap, in particular to see if there exists a
ConcurrentIdentityHashMap or if we can create one. The IdentityHashMap uses
System.identityHashCode(Object) instead of the overridden hashCode and ==
instead of equals. Even with Class instances as keys, this kind of Map should
perform well.
I also don't understand all the logic when caching the assignableTo info.
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
Seems like you would want to cache all instances of To and From, whether or not
the answer is True. Once anyone asks if two types are assignable, and you find
out the answer, why not cache the answer? From the code, it looks like you only
cache True results. What if you change the above code to:
Boolean isAssignableResult = Boolean.valueOf(isAssignable);
assignableTo.put(to, isAssignableResult);
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472506
]
Abe White commented on OPENJPA-141:
---
Craig thinks: Weak references are supposed to be cleaned up if the referenced
instance is not otherwise referenced. What would cause the referred classes
to be garbage collected immediately? I don't quite understand the issue here.
No, the current code holds the Classes with hard refs, and maps them to various
values with weak refs. Nothing else refers to these weak values. Therefore
they should be eligible for GC immediately, and their map entries should be
removed on the next map mutation (reference maps only clean up their expired
entries on mutation). That's why I don't understand how the current caches are
working at all to boost performance, unless GC isn't happening very often.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen profilers skew results in random ways like this) or even a
bug in ConcurrentReferenceHashMap.
The same goes for all the repeat logic in FetchConfigurationImpl.
And if we keep this code or some variant of it, I strongly suggest
moving it to a common place like ImplHelper.
+/**
+ * Generate the hashcode for this Id. Cache the type's
generated hashcode
+ * so
[jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
[
https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472510
]
Craig Russell commented on OPENJPA-141:
---
Craig thinks: Weak references are supposed to be cleaned up if the
referenced instance is not otherwise referenced. What would cause the
referred classes to be garbage collected immediately? I don't quite
understand the issue here.
No, the current code holds the Classes with hard refs, and maps them to
various values with weak refs. Nothing else refers to these weak values.
Therefore they should be eligible for GC immediately, and their map entries
should be removed on the next map mutation (reference maps only clean up their
expired entries on mutation). That's why I don't understand how the current
caches are working at all to boost performance, unless GC isn't happening very
often.
Roger that. Thanks for the explanation, I'm now confused as well. :-(
So what might work is having the keys be weak, and the otherwise unreferenced
values strong. That way, the values will always be there as long as the keys
are there. But if you do this, then you have to actually use the Class
instances as keys, which implies solving the hashCode/equals problem.
More performance improvements (in response to changes for OPENJPA-138)
--
Key: OPENJPA-141
URL: https://issues.apache.org/jira/browse/OPENJPA-141
Project: OpenJPA
Issue Type: Sub-task
Components: jpa
Reporter: Kevin Sutter
Assigned To: Kevin Sutter
Abe's response to my committed changes for OPENJPA-138. I will be working
with Abe and my performance team to work through these issues...
==
--- incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java (original)
+++ incubator/openjpa/trunk/openjpa-kernel/src/main/java/org/apache/
openjpa/ee/JNDIManagedRuntime.java Sun Feb 11 18:33:05 2007
@@ -29,6 +29,7 @@
implements ManagedRuntime {
private String _tmLoc = java:/TransactionManager;
+private static TransactionManager _tm;
Whoa, I didn't think you meant caching the TM statically. That has
to be backed out. You can cache it in an instance variable, but not
statically. Nothing should prevent someone having two different
BrokerFactories accessing two different TMs at two different JNDI
locations.
BrokerImpl:
+ * Cache from/to assignments to avoid Class.isAssignableFrom
overhead
+ * @param from the target Class
+ * @param to the Class to test
+ * @return true if the to class could be assigned to from
class
+ */
+private boolean isAssignable(Class from, Class to) {
+ boolean isAssignable;
+ ConcurrentReferenceHashMap assignableTo =
+ (ConcurrentReferenceHashMap) _assignableTypes.get(from);
+
+ if (assignableTo != null) { // to cache exists...
+ isAssignable = (assignableTo.get(to) != null);
+ if (!isAssignable) { // not in the map yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo.put(to, new Object());
+ }
+ }
+ } else { // no to cache yet...
+ isAssignable = from.isAssignableFrom(to);
+ if (isAssignable) {
+ assignableTo = new ConcurrentReferenceHashMap(
+ ReferenceMap.HARD, ReferenceMap.WEAK);
+ _assignableTypes.put(from, assignableTo);
+ assignableTo.put(to, new Object());
+ }
+ }
+ return isAssignable;
+}
This code could be simplified a lot. Also, I don't understand what
you're trying to do from a memory management perspective. For the
_assignableTypes member you've got the Class keys using hard refs and
the Map values using weak refs. No outside code references the Map
values, so all entries should be eligible for GC pretty much
immediately. The way reference hash maps work prevents them from
expunging stale entries except on mutators, but still... every time a
new entry is added, all the old entries should be getting GC'd and
removed. Same for the individual Map values, which again map a hard
class ref to an unreferenced object value with a weak ref. Basically
the whole map-of-maps system should never contain more than one entry
total after a GC run and a mutation.
I'd really like to see you run your tests under a different JVM,
because it seems to me like (a) this shouldn't be necessary in the
first place, and (b) if this is working, it's again only because of
some JVM particulars or GC timing particulars or testing particulars
(I've seen
