Re: [cp-patches] FYI: Implemented an ObjectPool

[Robert Schuster]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,
shouldn't the pooling class use a WeakHashMap?

And then I want to admit that the Apidoc should really explain when to
use a pooled object and when it is not good to use it. Eg. for all kind
of MouseEvent I think it is not a good idea to use the pool because the
user can access the Point instance and store it somewhere.

Putting such an object back in the pool (even if it was untouched in
Classpath API code) would sooner or later cause havoc (when the values
in the object are overwritten and the user did not expect that).

And now an example to get to know whether I have understood how to use
the object pool correctly:
In JTable I would change all instantiations of Point to
ObjectPool.getPoint(). When new values for these internal objects are
available I set them with begin.x, begin.y and so on.

When the JTable instance is freed (finalize) I return the Point
instances to the ObjectPool.

Done right?

cu
Robert

Roman Kennke wrote:
> Hi,
> 
> 
>>>I implemented and added an ObjectPool class.
>>
>>Please measure! Don't assume that this will lead to better performance.
>>I do follow your reasoning. But ultimately this is the job of the
>>allocator/garbage collector.
> 
> 
> I completely agree with you that ideally this should be done in the GC.
> This is why I hestitate to actually introduce some 'optimizations'. I
> don't know about the current situation in free VMs Garbage Collectors
> and how the impact would be. It would be helpful if some VM hackers
> could comment on the issue.
> 
> 
>>Please make it really easy to turn on/off caching so people can do real
>>measurements of the performance impact.
> 
> 
> I have added a flag in the ObjectPool that can turn off the cache. As
> well as some counters that count how many objects are
> requested/returned/created/pooled. This is not a very clever benchmark,
> since it does not actually measure performance (time-wise).
> 
> 
>> And only introduce it when it is
>>a clear win.
> 
> 
> Seeing that it is quite difficult to find the right spots where objects
> should actually be returned into the pool, I strongly hesitate now to
> introduce anything.
> 
> 
>>What seems to happen a lot in larger projects is that someone introduces
>>some pooling/caching of certain constructs and when some implementation
>>detail somewhere else in the stack changes the pooling/caching isn't
>>revised/remeasured to see if it still makes sense (or that it actually
>>decreases performance given the new circumstances!).
>>Don't let that happen in this case.
> 
> 
> I won't. It would be nice if some folks would play a little and comment
> on this issue. Note that the pool isn't actually in use in Classpath, it
> only sits around now for testing. I committed the following changes that
> could help benchmarking:
> 
> 2005-07-01  Roman Kennke  <address@hidden>
> 
>         * gnu/classpath/ObjectPool.java:
>         Introduced flag for turning on/off caching.
>         (getInstance): Synchronized access to this method.
>         (borrowObject): Synchronized access to the pool.
>         Added some benchmarking statements.
>         (returnObject): Synchronized access to the pool.
>         Added some benchmarking statements.
>         (createObject): Synchronized access to the pool.
>         Added some benchmarking statements.
>         (printStats): New method. Prints out some stats about the pool
>       usage.
> 
> /Roman
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: gnu/classpath/ObjectPool.java
> ===================================================================
> RCS file: /cvsroot/classpath/classpath/gnu/classpath/ObjectPool.java,v
> retrieving revision 1.2
> diff -u -r1.2 ObjectPool.java
> --- gnu/classpath/ObjectPool.java     1 Jul 2005 10:58:07 -0000       1.2
> +++ gnu/classpath/ObjectPool.java     1 Jul 2005 13:04:39 -0000
> @@ -77,6 +77,12 @@
>  public final class ObjectPool
>  {
>  
> +  /** The maximum number of instances that we keep for each type. */
> +  private static final int MAX_POOL_SIZE = 128;
> +
> +  /** This flag turns on/off caching (for benchmarking purposes). */
> +  private static final boolean IS_CACHING = true;
> +
>    /** The only instance of ObjectPool. */
>    private static ObjectPool instance;
>  
> @@ -87,6 +93,14 @@
>     * requested type is in the pool.
>     */
>    private HashMap pool;
> +  
> +  /**
> +   * Collect some stats in this fields. TODO: Can be removed later.
> +   */
> +  int created = 0;
> +  int requested = 0;
> +  int returned = 0;
> +  int pooled = 0;
>  
>    /**
>     * Creates a new instance of ObjectPool. This constructor is made private
> @@ -102,7 +116,7 @@
>     *
>     * @return an ObjectPool instance ready for use
>     */
> -  public static ObjectPool getInstance()
> +  public static synchronized ObjectPool getInstance()
>    {
>      if (instance == null)
>        instance = new ObjectPool();
> @@ -123,15 +137,32 @@
>     */
>    public Object borrowObject(Class type)
>    {
> +    // This is only here for benchmarking purposes.
> +    if (!IS_CACHING)
> +      return createObject(type);
> +    // Counts the requested objects. This is only here for benchmarking
> +    // purposes.
> +    requested++;
> +    if (requested % 10000 == 0)
> +      printStats();
> +
> +
>      Object object = null;
> -    Stack pooledInstances = (Stack) pool.get(type);
> +    Stack pooledInstances = null;
> +    synchronized (this)
> +      {
> +     pooledInstances = (Stack) pool.get(type);
> +      }
>      if (pooledInstances == null)
>        object = createObject(type);
>      else
>        if (pooledInstances.size() == 0)
>       object = createObject(type);
>        else
> -     object = pooledInstances.pop();
> +     synchronized (this)
> +       {
> +         object = pooledInstances.pop();
> +       }
>      return object;
>    }
>  
> @@ -142,14 +173,36 @@
>     */
>    public void returnObject(Object object)
>    {
> +    // This is only here for benchmarking purposes.
> +    if (!IS_CACHING)
> +      return;
> +    // Count the returned objects. This is only here for benchmarking 
> purposes.
> +    returned++;
> +
>      Class type = object.getClass();
> -    Stack pooledInstances = (Stack) pool.get(type);
> +    Stack pooledInstances = null;
> +    synchronized (this)
> +      {
> +     pooledInstances = (Stack) pool.get(type);
> +      }
>      if (pooledInstances == null)
>        {
>       pooledInstances = new Stack();
> -     pool.put(type, pooledInstances);
>        }
> -    pooledInstances.push(object);
> +    if (pooledInstances.size() < MAX_POOL_SIZE)
> +      synchronized (this)
> +     {
> +       pool.put(type, pooledInstances);
> +
> +       // Count the objects that are actually pooled. This is only
> +       // here for benchmarking purposes.
> +       pooled++;
> +     }
> +
> +    synchronized (this)
> +      {
> +     pooledInstances.push(object);
> +      }
>    }
>  
>    /**
> @@ -162,6 +215,11 @@
>     */
>    private Object createObject(Class type)
>    {
> +    // Counts the objects that are created here. This is only here for
> +    // benchmarking purposes.
> +    created++;
> +
> +
>      Object object = null;
>      try
>        {
> @@ -177,4 +235,16 @@
>        }
>      return object;
>    }
> +
> +  /**
> +   * This method prints out some stats about the object pool. This gives
> +   * an indication on how efficiently the pool is used.
> +   */
> +  void printStats()
> +  {
> +    System.err.println("Requested Objects: " + requested);
> +    System.err.println("Returned Objects: " + returned);
> +    System.err.println("Created Objects: " + created);
> +    System.err.println("Pooled Objects: " + pooled);
> +  }
>  }
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Classpath-patches mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/classpath-patches
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFCxb39G9cfwmwwEtoRAmRuAJwKU7jJWAYADyzeZK9fnf0rKdXtAgCdGR1l
ZvwAAcJPQFxUcpx48XKssVY=
=pfCQ
-----END PGP SIGNATURE-----