Hi, again!
2006/5/23, Antranig Basman <[EMAIL PROTECTED]>:
Oliver Zeigermann wrote:
>
> Hi Antranig!
Hi!
> 2006/5/23, Antranig Basman <[EMAIL PROTECTED]>:
> > The most straightforward of them is in GenericLockManager, where
> > the lock owner is not being released properly - I have
> > the following:
> >
> > Index:
> >
E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > ===================================================================
> > ---
> >
E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > (revision 408762)
> > +++
> >
E:/workspace/commons-transaction-test/src/java/org/apache/commons/transaction/locking/GenericLockManager.java
> > (working copy)
> > @@ -337,6 +337,8 @@
> > lock.release(ownerId);
> > locks.remove(lock);
> > }
> > + // AMB fix leak
> > + globalOwners.remove(ownerId);
> > }
> > }
>
> Hmmm. Do you argue that the owner can be removed from the list of
> owners as it no longer holds any locks at this particular point of
> time? That's what I presume.
>
> I wonder, is it possible that another thread acquires a new lock for
> that specific owner in the meantime? So that this removal would
> discard that. Which of course would be an error.
>
> Is that possible?
>
> Anyway, there really should be a way to get rid of an owner if it
> really is no longer needed. Maybe a new method for that?
OK, perhaps not even this leak is straighforward.
I'm worried about adding more APIs in this area since the
ownership/scoping rules are already difficult enough for me to
understand. If I understand your point correctly, this means that
what we require inside GLM is an "automated" scheme such that,
at *any point* where the last lock is removed for an owner,
bringing the set size down to zero, the entire globalOwners
entry is collected, in some kind of thread-safe way.
I.e. this condition, presumably, could even be triggered from
"removeOwner".
Perhaps a private/protected member entitled
"checkLocksEmpty(Object ownerId)" which will synchronize on
globalOwners, and then remove the Locks map for it if it is
empty?
Yeah, something like that, I guess.
This sounds expensive, since globalOwners would be a top-level
lock, but I can't think of a more clever scheme at the moment.
Expensive? I guess you mean in terms of possible concurrency, right?
In general I'm afraid of the very large amount of locking I see
going on throughout transaction, but understandably given it is
managing set of locks, quite a lot of this is probably
unavoidable(!).
However I think performance could be considerably improved if
there were to be a movement over to one of Doug Lea's kinds of
Concurrent Hashmaps, probably from his older "oswego" or
"backport" packages so we could maintain JDK 1.4 compliance.
Well, that actually may be the case. Maybe for the 2.0 release ;)
Anyway, I just could not figure out what the license for that code
was. Any idea?
What sort of load/throughput testing has been done on
commons-transaction?
Speaking only for myself load/throughput testing has been done for
commons transaction as part of Jakarta Slide. In that specific
scenario locking and transaction code has never been any bottleneck.
But Slide works on the database or the filesystem. No idea how that
would be if everything happens in memory.
> > If anyone is interested I could renew my efforts to find more
> > fine-grained patches - for example in the current version rev
> > 375638 there should definitely at line 1628 be a call to
> >
> > globalOpenResources.remove(is);
> >
> > following the call to
> > globalTransactions.remove(txId);
> >
> > I can't recall where the 3rd leak site was, but it was certainly
> > resolved by commenting out globalOpenResources.
>
> That seems reasonable to me. However, the whole topic is so
> complicated I wouldn't want to change that before more intense
> thinking. I would be pleased if you could have a closed look to those
> leaks and test the patches properly.
OK, I see that the globalOpenResources table is indeed required for
coherence. I will try to compose a patch with it incorporated that
does not leak - unfortunately I will be away for the next two
weeks and so will not be able to look at it very quickly.
Cool! Looking forward to those patches!
Cheers,
Oliver
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
