db-ojb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Armin Waibel" <armin.wai...@code-au-lait.de>
Subject Re: Some performance tuning changes
Date Tue, 13 May 2003 09:59:12 GMT
Hi Thomas,

> Hi all,
>
> thanks Armin for looking at this stuff. Unfortuantely I'm currently
very
> busy with a real-world project and won't find the the time to review
Lance's
> patches.
>
> But from reading through his mail I feel that they contain some
important
> improvements.

Indeed! It's really good stuff!

>
> The only thing I'm having problems with is step 1).
> A long time ago we dropped indexed access and started to use access by
name
> as we had a lot of problems with dynamic modification of class- and
> field-descriptors.
> If you remove a fielddescriptor and use indexed access you are lost!
> So any improvement in this area must take care nit to compromise
dynamic
> changes to the metaddata layer!

I don't know much about this part of OJB and it could
be critical to bustle around without knowing what to do ;-),
thus I delegate these unworked points to (you and) Jakob:

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

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

> 8) SqlHelper.cleanPath calls splitPath
> * ???
open


regards,
Armin

>
> cheers,
> Thomas
>
> > -----Original Message-----
> > From: Armin Waibel [mailto:armin.waibel@code-au-lait.de]
> > Sent: Monday, May 12, 2003 8: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 ...
> > * ???
> >
> > regards,
> > Armin
> >
> > ----- 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
> > RowReader:
> >
> >     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/
> > index
> >    RsIterator - call RowReader to build index for ResultSet in
> > constructor and call new readObjectArrayFrom
> >    SqlBasedRsIterator - call RowReader to build index for ResultSet
in
> > constructor
> >
> >
> >
> > 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:
> >    ClassDescriptor
> >       - changed map building strategy in
> > getXXXDescriptorByName methods
> >       - changed setClassOfObject and isAbstract to cache value of
> > isAbstract
> >       - 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
> > objects
> >    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
> > methods
> >    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
> > instead.
> >
> > 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
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: ojb-dev-unsubscribe@db.apache.org
> For additional commands, e-mail: ojb-dev-help@db.apache.org
>
>



Mime
View raw message