openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abe White (JIRA)" <j...@apache.org>
Subject [jira] Commented: (OPENJPA-141) More performance improvements (in response to changes for OPENJPA-138)
Date Thu, 15 Feb 2007 16:29:05 GMT

    [ 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.


Mime
View raw message