db-ojb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lance Eason" <lance.ea...@whisperwire.com>
Subject RE: Some performance tuning changes
Date Mon, 12 May 2003 21:55:37 GMT
Thank you for reviewing these.

Looking at PoorMansLoggerImpl it looks like isEnabed is implemented.  The profiler certainly
shows a difference:

          Calls       Cumulative Time       Cumulative Objects
Before:  27,071                 4,029                  163,833
After:   27,071                   812                      955

Actually reviewing this further though what causes all the calls is that RsIterator and ResultSetAndStatement
call getLogger() and cache it on a per instance basis.  It seem like it would be better if
the logger references in those two classes were made static instead.

-----Original Message-----
From: Armin Waibel [mailto:armin.waibel@code-au-lait.de]
Sent: Monday, May 12, 2003 1:37 PM
To: OJB Developers List
Subject: Re: Some performance tuning changes

Hi Lance,

thanks for your excellent improvements and the
detailed description.
Intermediate result:

[* ??? not yet done]

1) RowReaderDefaultImpl retrieves values from the....
* ???

2) Three methods on ClassDescriptor...
- OK, is done

3) ObjectCacheDefaultImpl and ObjectCachePerBrokerImpl both call
toString() on the Identity....
- OK, is done

4) LoggerFactoryImpl.getLogger concatenates a bunch of Strings ....
- Curently the used BootLogger instance (PoorMansLoggerImpl)
does not support isDebugEnabled/isEnabled methods, thus this
does not increase performance. Further on the log-statements
were called once per class, after it the logger was cached.

5, 6 and 7) PersistenceBrokerImpl is creating iterators to iterate ....
* ???

8) SqlHelper.cleanPath calls splitPath
* ???

9) PersistenceBrokerAbstractImpl manages firing off events during the
lifecycle ...
- OK, is done

10) ObjectReferenceDescriptor.getItemProxyForClass uses whether the
proxy ...
* ???


----- Original Message -----
From: "Lance Eason" <lance.eason@whisperwire.com>
To: "OJB Dev Mailing List (E-mail)" <ojb-dev@db.apache.org>
Sent: Monday, May 12, 2003 8:00 AM
Subject: Some performance tuning changes

I've been profiling my system with some large queries (returning
thousands of items each).  As a result I ended up making a few code
changes to tune OJB.  All told they improve OJB's performance by about
20% and reduce object creation by about the same for the scenarios I've
tested, nothing earth-shattering but every bit helps.  Only two of the
changes required any interface changes, but one of those is also the
single most significant change.

Could some committer review and add them?  All changes are made against
the latest from CVS as of this afternoon.  Because of mailer limitations
I'm sending the files as a follow-up to this message.

The changes I made, from most significant to least significant:

1) RowReaderDefaultImpl retrieves values from the result set via column
name instead of column index.  This quickly showed up as a hotspot in
the profiler.  While it's definitely a lot more robust to use the name
instead of the index there's no reason to pay the overhead of the name
lookup (which is significant) on each and every row.  The names can be
converted to indices once up front using ResultSet.findColumn(name) and
then values can be retrieved by index for each row.  Since the same
RowReader may service multiple RsIterators at the same time and
different RsIterators may have different indices for the same column,
the indices have to be local to the RsIterator somehow.  I went with a
sort of cookie approach where the RsIterator asks the RowReader to index
the columns and then hands that index back to the RowReader on the
readObjectArrayFrom method.  So I added two method signatures to

    public int[] index(ResultSet rs);
    public void readObjectArrayFrom(ResultSet rs, int[] index, Map row);

and changed RowReaderDefaultImpl, RsIterator and SqlBasedRsIterator
appropriately.  The other alternative would have been to instantiate a
RowReader for each RsIterator and keep this information inside the
RowReader itself, but that instantiation would have had a cost as well.
I left the original readObjectArrayFrom() signature as well and left the
PersistenceBroker.getObjectByIdentity() path using it since for single
item queries there is probably a very small performance advantage to the
original method.

Files modified:
   RowReader - added methods index and readObjectArrayFrom w/ index
   RowReaderDefaultImpl - implemented index and readObjectArrayFrom w/
   RsIterator - call RowReader to build index for ResultSet in
constructor and call new readObjectArrayFrom
   SqlBasedRsIterator - call RowReader to build index for ResultSet in

2) Three methods on ClassDescriptor, getFieldDescriptorByName,
getObjectReferenceDescriptorByName, and getCollectionDescriptorByName
all have the same fundamental problem.  They each try to build a
descriptor by name map lazily.  To do this they look for the name in the
map and if they don't find it scan through all the descriptors and if
found add it to the map.  The problem with this is that queries for
things where the descriptor doesn't exist always end up scanning the
entire set of descriptors in order to determine that it doesn't exist.
In other words this strategy works fine for positive queries but is very
poor for negative queries.  It turns out that a fair number of negative
queries do occur though for things like the field descriptor for
OJB_CONCRETE_CLASS.  I also fixed a bug where ClassDescriptor doesn't
rebuild it's cached lists of FieldDescriptors when new field descriptors
are added and I cached the result of isAbstract.

Files modified:
      - changed map building strategy in getXXXDescriptorByName methods
      - changed setClassOfObject and isAbstract to cache value of
      - changed addFieldDescriptor to clear out cached lists of field
descriptors and clear the name map
      - changed addCollectionDescriptor and addObjectReferenceDescriptor
to clear their respective name maps

3) ObjectCacheDefaultImpl and ObjectCachePerBrokerImpl both call
toString() on the Identity passed in and use that as the key to their
internal maps.  In addition Identity calls toString() on itself to
calculate it's hashCode.  Converting the Identity objects to Strings
turns out to be much more costly than simply comparing their contents.
Compounding matters is the fact that a lot of Identity objects get
constructed solely to do cache lookups and then are immediately
discarded but they get unnecessarily converted to String in the process
at extra cost in processing and object creation.  I changed the two
caches to use the Identity object itself as the key and changed
Identity.hashCode() to calculate the hashCode directly rather than
converting to a String.

Files modified:
   Identity - changed hashCode() method
   ObjectCacheDefaultImpl - removed all the toString()'s on Identity
   ObjectCachePerBrokerImpl - ditto

4) LoggerFactoryImpl.getLogger concatenates a bunch of Strings for debug
messages without checking to see if debug verbosity is enabled.  I
protected all the calls with an isEnabledFor(Logger.DEBUG) check.

Files modified:
   LoggerFactoryImpl - check isEnabledFor(Logger.DEBUG) before doing
String concatenations

5, 6 and 7) PersistenceBrokerImpl is creating iterators to iterate
through Vectors returned from ClassDescriptor methods.  I changed it to
get elements by index instead (which is significantly faster).  Even
better would be if the ClassDescriptor methods returned arrays of the
appropriate type instead of Vectors (this would be more consistent with
the rest of the ClassDescriptor interface as well as performing better
and being more type-safe), but this would've required an interface
change to ClassDescriptor.

Also a lot of logger.debug messages were added to getRsIteratorFromQuery
that are doing String concatenations.  I protected all these calls with
logger.isDebugEnabled() checks.

When returning a collection proxy PersistenceBrokerImpl/ProxyHelper
always looks up the constructor again each time though the answer never
changes.  I changed it to remember the constructors and use those
instead of looking up the constructors from the classes each time.

Files modified:
   PersistenceBrokerImpl - replaced iterators with get elements by
index, remembered proxy collection constructors and protected
logger.debug messages
   ProxyHelper - remember constructors for standard proxy collections

8) SqlHelper.cleanPath calls splitPath.  This results in lots of
PathInfo objects being created needlessly and immediately discarded
because cleanPath is only interested in the column portion.  I changed
it to directly calculate the column portion itself and to avoid two
indexOf operations in both cleanPath and splitPath when unnecessary.

Files modified:
   SqlHelper - optimized cleanPath and splitPath

9) PersistenceBrokerAbstractImpl manages firing off events during the
lifecycle of a PersistenceBroker.  There are three opportunities for
performance improvements here.  First it iterates through a map every
time an event is fired.  Since listeners are added rarely but events are
fired frequently it's much better to construct a new array each time a
listener is added and iterate through an array on event firing.  Second,
listeners are stored in a map with a Boolean indicating whether they are
permanent or not.  When the broker is released this map is iterated
through and all non-permanent listeners are discarded.  It's better to
keep two separate arrays and just discard the non-permanent array rather
than doing this iteration.  Finally, a single fireBrokerEvent method is
provided which takes a PersistenceBrokerEvent.  Internally this method
does two instanceof operations to decide how to dispatch the event.
Letting the compiler decide how to dispatch at compile time by providing
overloaded signatures of fireBrokerEvent is more efficient.  Similar
things could be done for add and removeListener, but I didn't bother
since these will be called relatively rarely.

Files modified:
   PersistenceBrokerAbstractImpl - changed storage of listeners to
arrays instead of Maps, added overloaded fireBrokerEvent methods
   PersistenceBroker - added overloaded fireBrokerEvent methods
   DelegatingPersistenceBroker - added overloaded fireBrokerEvent
   PersistenceBrokerBean - added overloaded fireBrokerEvent methods

10) ObjectReferenceDescriptor.getItemProxyForClass uses whether the
proxy is null or not as a test for whether it's been looked up.  For
classes that don't have a proxy though the proxy will always be null so
it will do that lookup again every time the method is called.  I changed
it to have a boolean indicating whether the proxy had been looked up

Files modified:
   ObjectReferenceDescriptor - changed getItemProxyForClass to use a
boolean to indicate whether the proxy has been looked up

To unsubscribe, e-mail: ojb-dev-unsubscribe@db.apache.org
For additional commands, e-mail: ojb-dev-help@db.apache.org

To unsubscribe, e-mail: ojb-dev-unsubscribe@db.apache.org
For additional commands, e-mail: ojb-dev-help@db.apache.org

View raw message