I am +0 for replacing the synchronized pools in PoolUtils with this approach. The "0" is because the impls are already there and may perform better (quite possibly not), the "+" because there is less code in the proxy impl. I am also not 100% convinced that we should be providing these at all. There is a warning to the user in the javadoc that we should keep if we keep these. Interested in others' thoughts on this.
Phil On Tue, Dec 28, 2010 at 11:36 AM, Simone Tripodi <simone.trip...@gmail.com>wrote: > Hi again guys, > just realized that pasted code is not so readable, I just copied it on > pastie[1]. > If you don't have any objections, I can start committing that stuff so > you can review it. > Many thanks in advance, > Simo > > [1] http://pastie.org/1411379 > > http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> > http://www.99soft.org/ > > > > On Sun, Dec 26, 2010 at 1:41 PM, Simone Tripodi > <simone.trip...@gmail.com> wrote: > > Hi Phil, > > that's the way how I propose to synchronize pools using proxies > > without implementing synchronized wrapper classes, if you don't see > > issues about it I can start committing it let you all reviewing. > > Please let me know, have a nice day!!! > > Simo > > > > {{{ > > > > public static <T> ObjectPool<T> synchronizedPool(final ObjectPool<T> > pool) { > > return synchronizedObject(pool, ObjectPool.class); > > } > > > > public static <K,V> KeyedObjectPool<K,V> synchronizedPool(final > > KeyedObjectPool<K,V> keyedPool) { > > return synchronizedObject(keyedPool, KeyedObjectPool.class); > > } > > > > public static <T> PoolableObjectFactory<T> > > synchronizedPoolableFactory(final PoolableObjectFactory<T> factory) { > > return synchronizedObject(factory, PoolableObjectFactory.class); > > } > > > > public static <K,V> KeyedPoolableObjectFactory<K,V> > > synchronizedPoolableFactory(final KeyedPoolableObjectFactory<K,V> > > keyedFactory) { > > return synchronizedObject(keyedFactory, > > KeyedPoolableObjectFactory.class); > > } > > > > private static <T> T synchronizedObject(final T toBeSynchronized, > > final Class<T> type) { > > if (toBeSynchronized == null) { > > throw new IllegalArgumentException("Object has to be > > synchronized must not be null."); > > } > > > > /** > > * Used to synchronize method declared on the pool/factory > > interface only. > > */ > > final Set<Method> synchronizedMethods = new HashSet<Method>(); > > for (Method method : type.getDeclaredMethods()) { > > synchronizedMethods.add(method); > > } > > > > return type.cast(Proxy.newProxyInstance(type.getClassLoader(), > > new Class<?>[] { type }, > > new InvocationHandler() { > > > > private final Object lock = new Object(); > > > > public Object invoke(Object proxy, Method method, > > Object[] args) throws Throwable { > > if (synchronizedMethods.contains(method)) { > > synchronized (this.lock) { > > return method.invoke(toBeSynchronized, > args); > > } > > } > > return method.invoke(toBeSynchronized, args); > > } > > > > } > > )); > > } > > > > }}} > > > > http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> > > http://www.99soft.org/ > > > > > > > > On Fri, Dec 24, 2010 at 3:32 PM, Simone Tripodi > > <simone.trip...@gmail.com> wrote: > >> Hi Phil, > >> thanks a lot for feedbacks and for mentoring, much more than > appreciated. > >> I take advantage to wish you all a Merry Christmas, all the best, > >> Simo > >> > >> http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> > >> http://www.99soft.org/ > >> > >> > >> > >> On Fri, Dec 24, 2010 at 4:32 AM, Phil Steitz <phil.ste...@gmail.com> > wrote: > >>> On Thu, Dec 23, 2010 at 2:28 AM, Simone Tripodi < > simone.trip...@gmail.com>wrote: > >>> > >>>> Hi Phil, > >>>> > >>>> >> > >>>> >> org.apache.commons.pool2.impl > >>>> >> |---- generic > >>>> >> |---- reference > >>>> >> |---- stack > >>>> >> > >>>> >> common stuff could be included directly under impl. > >>>> >> > >>>> > > >>>> > What exactly would that be? > >>>> > > >>>> > >>>> just realized that there are no common stuff shared by different kind > of > >>>> pool :P > >>>> > >>>> > > >>>> > I was going to propose dropping the stack pools altogether. The > >>>> LIFO/FIFO > >>>> > config option in the generic pools makes them mostly irrelevant > (i.e., > >>>> you > >>>> > can get the same behavior with a suitably configured GOP / GKOP with > much > >>>> > more configurability) > >>>> > > >>>> > >>>> I had the same feelings here, but felt a little shy on saying that. > >>>> > >>> > >>> No reason for that :) > >>> > >>> > >>>> You've my full support on this, I agree on dropping stack based pool > >>>> implementations. > >>>> > >>>> Good. Lets do it. > >>> > >>> > >>>> > I don't want to sound too conservative and I will certainly not > stand in > >>>> the > >>>> > way of new / different pool implementations, but I would personally > >>>> prefer > >>>> > to keep the number of included pool impls as small as possible. > >>>> > > >>>> > >>>> I propose a more democratic way, I mean, like you made us notice, > >>>> keeping/adding the pool impl only if it makes sense to. > >>>> I wouldn't think about the included pools in therms of "size" but > >>>> rather in therms of "meaning" > >>>> > >>> > >>> Agree this is completely up to the community and them who bring the > >>> patches. The reason that I tend to be conservative is that more impls > mean > >>> more bugs, which makes fixing them all and getting releases out quickly > >>> harder. Pooling code is tricky to maintain, so fewer pools with > >>> less-exotic features might be better all around - not just in terms of > >>> maintenance, but also approachability from the standpoint of users and > >>> contributors. > >>> > >>>> > >>>> > I think the first thing we need to do is to decide what > implementations > >>>> we > >>>> > are going to a) keep or b) add for 2.0. I have been convinced that > we > >>>> need > >>>> > to keep GKOP as well as GOP. As I said above, I would like to > consider > >>>> > dropping the stack-based pools. I think we should keep the > >>>> reference-based > >>>> > pool and I am open to the new ones you suggest, just don't have use > cases > >>>> > myself for them. > >>>> > >>>> I'm not ready to show use cases too, sorry :( But they can be added > >>>> with a trivial refactory, moving the current SoftReferenceObjectPool > >>>> implementation to an > >>>> > >>>> AbstractReferenceObjectPool<T, R extends Reference<T>> extends > >>>> BaseObjectPool<T> implements ObjectPool<T> > >>>> > >>>> then in subclasses do the minimum. I can quickly provide a patch for > it. > >>>> > >>> > >>> I am OK with this as long as it does not complicate the code too much. > >>> > >>>> > >>>> > There are quite a few impls buried in PoolUtils that might > >>>> > make sense to pull out (or eliminate). > >>>> > > >>>> > >>>> I just made a census (with proposals): > >>>> > >>>> * PoolableObjectFactoryAdaptor > >>>> * KeyedPoolableObjectFactoryAdaptor > >>>> * ObjectPoolAdaptor > >>>> * KeyedObjectPoolAdaptor > >>>> > >>>> I propose to eliminate these adaptors, > >>> > >>> > >>> That's the spirit - he he > >>> > >>> > >>>> they implement a behavior that > >>>> users can replicate with a trivial code and without build a > >>>> pool/factory on top of an existing ones > >>>> > >>>> * CheckedObjectPool > >>>> * CheckedKeyedObjectPool > >>>> > >>>> These can be eliminated too, having introduced the generics > >>>> > >>> > >>> +1 - I think Gary may have mentioned this already. Lets get rid of > them. > >>> > >>>> > >>>> * ObjectPoolMinIdleTimerTask > >>>> * KeyedObjectPoolMinIdleTimerTask > >>>> > >>>> I propose these pools can be pulled out and moved to a proper package > >>>> > >>>> I wonder if anyone uses these? I wonder also if it might be better to > just > >>> expose ensureMinIdle. > >>> > >>> > >>>> * SynchronizedObjectPool > >>>> * SynchronizedKeyedObjectPool > >>>> * SynchronizedPoolableObjectFactory > >>>> * SynchronizedKeyedPoolableObjectFactory > >>>> > >>>> These could be pulled out too, even if something suggests me that > >>>> pools synchronization can be realized with just a Proxy, I'll do a > >>>> little experiment to submit so you can evaluate. > >>>> > >>> > >>> Interested in ideas on this. Here again, I am not sure how much use > these > >>> actually get. > >>> > >>>> > >>>> > What might make sense is to replace "impl" with "instance" (or > "object") > >>>> and > >>>> > "reference" (or "ref"). So you have o.a.c.p, o.a.c.p.instance, > >>>> > o.a.c.p.reference. > >>>> > > >>>> > >>>> sounds much better than keeping the intermediate "impl", I agree :) > >>>> > >>>> Should I have to write all these notes on the wiki and open issues > >>>> before proceeding? > >>>> > >>> > >>> I would say wait a bit to see if anyone has problems with above and if > not, > >>> go ahead and make the changes. > >>> > >>> > >>>> Many thanks in advance, have a nice day! > >>>> > >>> > >>> Thank you! > >>> > >>> Phil > >>> > >>>> Simo > >>>> > >>>> --------------------------------------------------------------------- > >>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > >>>> For additional commands, e-mail: dev-h...@commons.apache.org > >>>> > >>>> > >>> > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
