Olav, Important point below...
Olav Sandstaa wrote:
> Hi,
>
> Just sharing some information if someone is interested or feel for
> commenting - feedback is appreciated (but feel free to press the delete
> (or power) button too):
>
> One of the uses of the old (as well as the new) dependency manager is to
> determine when it is safe to call Transaction::commitRecords() (and in
> most cases also being able to delete/release the transaction object
> itself). When using the old dependency manager one of the requirement
> was that the dependency counter had reached zero before calling
> Transaction::commitRecords().
>
> In the code using the old dependency manager
> Transaction::commitRecords() could be called in four/five different places:
>
> 1. Transaction::commit()
>
> 2 a+b). Transaction::releaseDependency() called either from
> Transaction::expungeTransaction() or Transaction::releaseDependencies().
>
> 3. Transaction::writeComplete() (callback from serial log/gopher thread)
>
> 4. TransactionManager::purgeTransaction() (clean-up of transaction by
> the scavenger).
>
> In the initial patch for the new dependency manager
> Transaction::commitRecords is called only from one place:
> Transaction::purgeTransaction() (case 4 above), i.e. it is not called
> until the Scavenger comes around (about every 30 seconds) [1]. Case 2a+b
> has been eliminated by removing the old dependency manager code. The
> code for case 1 and 3 have been temporarily commented out with a comment
> that "this code will be fixed by the next patch" :-) . Still, with only
> case 4 in the code it seems that Falcon works both correctly and "nice"
> but it might not be optimal to delay all calls to
> Transaction::commitRecords() until the scavenger comes around.
>
> So to get some data about which of the above cases that actually are in
> use I instrumented the code and ran the falcon test suite. The result is
> (number of calls to commitRecords()):
>
> Transaction::commit: 1074
> Transaction::releaseDependency: 10839
> Transaction::writeComplete: 179389
> TransactionManager::purgeTransaction: 168713
>
> Some more instrumentation revealed that for most Transactions
> commitRecords() was called twice. And further, in the table above *all*
> calls done by Transaction::commit() and
> TransactionManager::purgeTransactions() actually was a "second" call to
> Transaction::commitRecords().
>
> This leaves me believe/suggest the following for the new dependency
> manager for how and when to call commitRecords() (and delete committed
> transaction objects):
>
> * Case 1 above (in Transaction::commit()) is not necessary at all -
> remove it.
>
> * Case 2a+b: is already gone since the old dependency manger code is gone
>
> * Case 3: is very useful but in order to re-implement the "old
> dependency check" that just checks that the dependencies counter is
> zero, with the new dependency manager I would need to introduce a shared
> lock on the active transaction list to get the "startEvent" (also now
> known as the transactionId) of the oldest active transaction in order to
> do the "new dependency check". This does not seem like the best thing to
> do.
>
> So instead (based on a suggestion from Jim) I think the best (and
> easiest) way to call Transaction::commitRecords() and delete old
> transaction objects will be to do something similar to what the new
> implementation of TransactionManager::purgeTransaction() does each time
> we commit a record when we anyway have the exclusive lock on both the
> active and committed lists. This code will call commitRecords() and
> release the transaction objects from the front of the committed
> transaction list as long as they have a commitId (commitEvent) that is
> smaller than the transactionId (startEvent) of the oldest active
> transactions. The cost per transaction object should be very "similar"
> to the cost of doing the same in the scavenger.
You need to get at least a shared lock on
TransactionManager::activeTransactions to find the oldest active
transaction.
In your code you do this in purgeTransactions();
oldestActive = activeTransactions.first->startEvent;
The old code used a shared lock in
TransactionManager::findOldestActive() which called
TransactionManager::findOldest().
You may need to use TransactionManager::findOldest() which does get that
shared lock. It scans the whole list because is assumes that there are
some transactions in that list that have status=available. Your current
patch does not have and 'available' transactions in the active list, but
it will when you add back in commitNoUpdates and allocations of 10 at a
time.
Kevin
>
> * Case 4: TransactionManager::purgeTransaction() can probably be deleted
> (if I implement what suggested above).
>
> One extra benefit of this that instead of having five code paths that
> lead to commitRecords() in the old implementation, the new
> implementation will be simpler with commitRecords() only called in a
> single place.
>
> Everyone that is still reading and think they understand what I wrote:
> feel free to comment as I might have misunderstood or jumped to wrong
> conclusions. Everyone else: feel free to ask questions :-)
>
> Thanks for listening! (and yes, I will update the worklog....)
>
> Olav
>
> [1] The "record number" of committed transaction ready for clean-up by
> the scavenger I have seen in one of my tests is 330.000 transactions :-)
> (but that will not be typical because in the initial patch also
> read-only transactions ends up in the committed list)
>
>
>
