You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by "Kevin Sutter (JIRA)" <ji...@apache.org> on 2007/02/12 18:39:05 UTC
[jira] Created: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
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 (typeHashInt == null) {
> + Class base = type;
> + Class superclass = base.getSuperclass();
> + while (superclass != null && superclass !=
> Object.class) {
> + base = base.getSuperclass();
> + superclass = base.getSuperclass();
> + }
> + _typeHash = base.hashCode();
> + _typeCache.put(type, new Integer(_typeHash));
> + } else {
> + _typeHash = typeHashInt.intValue();
> + }
> }
> return _typeHash ^ idHash();
> }
Once again, you're mapping a hard Class ref to a value with no
outside references held in a weak ref. Once again that means the
entry should be immediately eligible for GC, and therefore should be
removed on the next mutation of the cache, subject to GC timing. And
again I'd like to know what your JVM is doing to make Class.hashCode
take an appreciable amount of time. Aren't Class instances supposed
to be singletons? What if we just used System.identityHashCode(cls)?
> Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> apache/openjpa/lib/conf/ObjectValue.java
> URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> view=diff&rev=506230&r1=506229&r2=506230
> ======================================================================
> ========
> --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> openjpa/lib/conf/ObjectValue.java (original)
> +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> @@ -17,6 +17,8 @@
>
> import org.apache.commons.lang.ObjectUtils;
> import org.apache.openjpa.lib.util.Localizer;
> +import org.apache.openjpa.lib.util.ReferenceMap;
> +import
> org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
>
> /**
> * An object {@link Value}.
> @@ -28,6 +30,10 @@
> private static final Localizer _loc = Localizer.forPackage
> (ObjectValue.class);
>
> + // cache the types' classloader
> + private static ConcurrentReferenceHashMap _classloaderCache =
> + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> ReferenceMap.WEAK);
This maps a hard Class ref to a weak ClassLoader ref. Given that a
Class references its ClassLoader (or is supposed to -- again I wonder
what the hell the JVM you're using is doing where
Class.getClassLoader is taking a long time), no entries will ever
expire from this map.
Have you tried running your benchmarks without all the caching of
assignables and classloaders and hashcodes (all Class methods, btw)
and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
Posted by Kevin Sutter <kw...@gmail.com>.
Exactly.
On 2/13/07, Patrick Linskey (JIRA) <ji...@apache.org> 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 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) {
> > > + Class base = type;
> > > + Class superclass = base.getSuperclass();
> > > + while (superclass != null && superclass !=
> > > Object.class) {
> > > + base = base.getSuperclass();
> > > + superclass = base.getSuperclass();
> > > + }
> > > + _typeHash = base.hashCode();
> > > + _typeCache.put(type, new Integer(_typeHash));
> > > + } else {
> > > + _typeHash = typeHashInt.intValue();
> > > + }
> > > }
> > > return _typeHash ^ idHash();
> > > }
> > Once again, you're mapping a hard Class ref to a value with no
> > outside references held in a weak ref. Once again that means the
> > entry should be immediately eligible for GC, and therefore should be
> > removed on the next mutation of the cache, subject to GC timing. And
> > again I'd like to know what your JVM is doing to make Class.hashCode
> > take an appreciable amount of time. Aren't Class instances supposed
> > to be singletons? What if we just used System.identityHashCode(cls)?
> > > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > > apache/openjpa/lib/conf/ObjectValue.java
> > > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > > view=diff&rev=506230&r1=506229&r2=506230
> > > ======================================================================
> > > ========
> > > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > > openjpa/lib/conf/ObjectValue.java (original)
> > > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > > @@ -17,6 +17,8 @@
> > >
> > > import org.apache.commons.lang.ObjectUtils;
> > > import org.apache.openjpa.lib.util.Localizer;
> > > +import org.apache.openjpa.lib.util.ReferenceMap;
> > > +import
> > > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> > >
> > > /**
> > > * An object {@link Value}.
> > > @@ -28,6 +30,10 @@
> > > private static final Localizer _loc = Localizer.forPackage
> > > (ObjectValue.class);
> > >
> > > + // cache the types' classloader
> > > + private static ConcurrentReferenceHashMap _classloaderCache =
> > > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > > ReferenceMap.WEAK);
> > This maps a hard Class ref to a weak ClassLoader ref. Given that a
> > Class references its ClassLoader (or is supposed to -- again I wonder
> > what the hell the JVM you're using is doing where
> > Class.getClassLoader is taking a long time), no entries will ever
> > expire from this map.
> > Have you tried running your benchmarks without all the caching of
> > assignables and classloaders and hashcodes (all Class methods, btw)
> > and just the other improvements? Or with any other JVM?
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ 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() != null
> > - && base.getSuperclass() != Object.class)
> > - base = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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) 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472674 ]
Kevin Sutter commented on OPENJPA-141:
--------------------------------------
> 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.
It's not that the GC isn't happening very often, it's that the transaction throughput is very fast (1500 per second). So, even if we lose an entry once in a while via the GC (once every 20 seconds or so), it's not that big of a deal to re-populate the cache. It's still a heck of lot better than performing the expensive operations on every invocation. And, changing these Weak references to Soft should even be better since the GC would only collect these references if memory is getting tight. But, either way is still better than what we have today.
> 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 (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ 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) {
> > + 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473521 ]
Kevin Sutter commented on OPENJPA-141:
--------------------------------------
> Looks good to me. Super-tiny nit: you've got a brace on a newline in an if statement in BrokerImpl.java, instead of the new brace-on-same-line convention that we've been using on OpenJPA.
That's because I ran up against the other OpenJPA convention of limiting the line length to 80 characters. If I divided up the conditional, it was difficult to read. And, the previous conditional statement in this same method did the same thing with dropping the brace to the next line, so I thought it was a safe alternative. ;-)
Thanks for reviewing the changes. If I don't hear anything else, I'll commit the changes later this evening.
> 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473231 ]
Craig Russell commented on OPENJPA-141:
---------------------------------------
1. The original functionality of isAssignable(Class a, Class b) is to return true if either class is assignable to the other. e.g.
/**
* Whether either of the two types is assignable from the other.
*/
private static boolean isAssignable(Class c1, Class c2) {
return c1 != null && c2 != null
&& (c1.isAssignableFrom(c2) || c2.isAssignableFrom(c1));
}
The new method doesn't seem to include both sides of the assignable functionality. Was this done on purpose?
> 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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
> _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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473445 ]
Kevin Sutter commented on OPENJPA-141:
--------------------------------------
Getting close to another patch posting...
>> 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.
>From what I can tell, it looks like the double check within FetchConfigurationImpl was overhead and not needed. Thus, my proposal is to just have the new helper method do the one-way check. And, leave FetchConfigurationImpl usage with single invocations (BrokerImpl usage was already one-way). Anybody with further historical knowledge in this area can surely pipe in... 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
> 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ 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 "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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Sutter resolved OPENJPA-141.
----------------------------------
Resolution: Fixed
Code changes complete per comments in 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, 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12472516 ]
Kevin Sutter commented on OPENJPA-141:
--------------------------------------
> 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.
Solving the hashcode/equals problem? You mean your concern about these operations being slow? At least this caching is faster than repeating the calls over and over again... :-)
It also seems that the ConcurrentReferenceHashMaps require at least one HARD reference, so maybe your suggestion of WEAK/HARD is something to look at. We're checking into it.
Thanks for the other suggestions on the isAssignable method as well.
More tomorrow...
> 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 (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ 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 = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by Craig L Russell <Cr...@Sun.COM>.
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:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ 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();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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. 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473253 ]
Patrick Linskey commented on OPENJPA-141:
-----------------------------------------
> > 4. Why are you using a static JNDI location -> TM cache in JNDITransactionManager
> > rather than just caching the TM in an instance variable?
>
> It's probably six of one, half a dozen of the other. I can make the change to use an
> instance variable.
Personally, I'm allergic to static fields, and can only tolerate them in small doses. So I prefer the instance variable caching. The nice thing about the OpenJPA configuration architecture is that the Configuration instance is a natural singleton for a given persistence unit; the more we can tie caches to the persistence unit, the better things will work in unexpected heterogeneous environments, during redeploys, etc.
> 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Sutter updated OPENJPA-141:
---------------------------------
Attachment: openjpa-141.txt
On second thought, I have attached the proposed patch to this Issue. Please review and provide feedback in a timely manner. I would like to commit these changes as soon as possible. Thank you.
The one thing that I decided to do is to go with SOFT references for the keys instead of WEAK. This way, in case the GC does start to run more often, it will only clean up these SOFT references if memory constraints exist.
Of course, all of the OpenJPA tests run just fine and the performance numbers are looking better.
Thanks for your help,
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
> 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ 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) {
> > + 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473515 ]
Patrick Linskey commented on OPENJPA-141:
-----------------------------------------
Looks good to me. Super-tiny nit: you've got a brace on a newline in an if statement in BrokerImpl.java, instead of the new brace-on-same-line convention that we've been using on OpenJPA.
> 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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 (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Patrick Linskey (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12473290 ]
Patrick Linskey commented on OPENJPA-141:
-----------------------------------------
> This is also affected by whether we use a giant cache (including all the EMFs) or one cache per EMF.
As I mentioned earlier, I generally prefer putting things in a Configuration or BrokerFactory to creating static fields. Now that I've actually looked at the code (!), I noticed that we only call the new ImplHelper method from places where we have a BrokerFactory handy. Maybe we should move the cache to the BrokerFactory or the Configuration? That way, at undeploy time (assuming that undeploy throws away the BrokerFactory), we don't need to do anything at all.
This could end up looking like a new method Configuration.isAssignable(Class from, Class to).
That said, in this case, I don't particularly mind using a static field, since this particular bit of data is already static (insofar as Class objects are singletons).
> 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 = base.getSuperclass();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Kevin Sutter (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/OPENJPA-141?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Sutter updated OPENJPA-141:
---------------------------------
Attachment: openjpa-141.txt
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... I have more performance-related items to pursue... :-) 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
> 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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 = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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();
> > - _typeHash = base.hashCode();
> > + Integer typeHashInt = (Integer) _typeCache.get(type);
> > + if (typeHashInt == null) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Craig Russell (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (OPENJPA-141) More performance improvements (in
response to changes for OPENJPA-138)
Posted by "Abe White (JIRA)" <ji...@apache.org>.
[ 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 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) {
> > + Class base = type;
> > + Class superclass = base.getSuperclass();
> > + while (superclass != null && superclass !=
> > Object.class) {
> > + base = base.getSuperclass();
> > + superclass = base.getSuperclass();
> > + }
> > + _typeHash = base.hashCode();
> > + _typeCache.put(type, new Integer(_typeHash));
> > + } else {
> > + _typeHash = typeHashInt.intValue();
> > + }
> > }
> > return _typeHash ^ idHash();
> > }
> Once again, you're mapping a hard Class ref to a value with no
> outside references held in a weak ref. Once again that means the
> entry should be immediately eligible for GC, and therefore should be
> removed on the next mutation of the cache, subject to GC timing. And
> again I'd like to know what your JVM is doing to make Class.hashCode
> take an appreciable amount of time. Aren't Class instances supposed
> to be singletons? What if we just used System.identityHashCode(cls)?
> > Modified: incubator/openjpa/trunk/openjpa-lib/src/main/java/org/
> > apache/openjpa/lib/conf/ObjectValue.java
> > URL: http://svn.apache.org/viewvc/incubator/openjpa/trunk/openjpa-
> > lib/src/main/java/org/apache/openjpa/lib/conf/ObjectValue.java?
> > view=diff&rev=506230&r1=506229&r2=506230
> > ======================================================================
> > ========
> > --- incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java (original)
> > +++ incubator/openjpa/trunk/openjpa-lib/src/main/java/org/apache/
> > openjpa/lib/conf/ObjectValue.java Sun Feb 11 18:33:05 2007
> > @@ -17,6 +17,8 @@
> >
> > import org.apache.commons.lang.ObjectUtils;
> > import org.apache.openjpa.lib.util.Localizer;
> > +import org.apache.openjpa.lib.util.ReferenceMap;
> > +import
> > org.apache.openjpa.lib.util.concurrent.ConcurrentReferenceHashMap;
> >
> > /**
> > * An object {@link Value}.
> > @@ -28,6 +30,10 @@
> > private static final Localizer _loc = Localizer.forPackage
> > (ObjectValue.class);
> >
> > + // cache the types' classloader
> > + private static ConcurrentReferenceHashMap _classloaderCache =
> > + new ConcurrentReferenceHashMap(ReferenceMap.HARD,
> > ReferenceMap.WEAK);
> This maps a hard Class ref to a weak ClassLoader ref. Given that a
> Class references its ClassLoader (or is supposed to -- again I wonder
> what the hell the JVM you're using is doing where
> Class.getClassLoader is taking a long time), no entries will ever
> expire from this map.
> Have you tried running your benchmarks without all the caching of
> assignables and classloaders and hashcodes (all Class methods, btw)
> and just the other improvements? Or with any other JVM?
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.