Kevin Lewis wrote:
> Olav, Important point below...
> Olav Sandstaa wrote:
>> 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
>> 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) . 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
>> * 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
Yes, I am aware of that. And that is my main argument for suggesting to
remove the current check if we can call Transaction::commitRecords()
from Transaction::writeComplete(). Do you think it is OK to remove the
call to Transaction::commitRecords() from Transaction::writeComplete()?
> In your code you do this in purgeTransactions();
> oldestActive = activeTransactions.first->startEvent;
Correct. But only after I have I have gotten a shared lock on
TransactionManager::activeTransactions. So I think this is safe this far.
> The old code used a shared lock in
> TransactionManager::findOldestActive() which called
> 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.
Yes, I agree that this code will have to be extended if/when there can
be "available" transaction objects in the (beginning of) the list. The
problem with this is that it increases the cost of finding the "oldest
active". Jim's original proposal was to have the active list sorted with
the oldest active at the front of the list.
Today I have implemented a patch for what I suggested in the email I
sent yesterday (as described above). Basically, I have moved the call to
TransactionManager::purgeTransactions() from being called by the
scavenger to being called by Transaction::commit(). The two main reasons
for this. The first is to call Transaction::commitRecords() and delete
old transaction commits much earlier than when using the scavenger for
this. The second reason is that in Transaction::commit() we anyway have
the necessary locks set on the active and committed transaction lists.
The cost of this strategy will be slightly more costly than having this
done as a "batch" by the scavenger but we will free up memory used for
records and transaction objects much earlier. And if we have to do a
search for the oldest active in the active list this will further
increase the cost slightly.
I think the main alternatives for calling Transaction::commitRecords()
and deleting old transactions objects are:
1. Transaction::commit() calls TransactionManager::purgeTransactions()
2. Transaction::writeComplete calls Transaction::commitRecords()
3. The scavenger calls TransactionManager::purgeTransactions()
With the old dependency manager, this was mostly done by a combination
of 2 and 3. The initial implementation for the new dependency manger do
it by using only 3. While my latest patch (which I hope to "commit"
today) changes this to alternative 1.
I am still thinking about the best ways to get the cost down by
preloading and possibly being able to re-use transaction objects in the
active list without making it costly to find the "oldest active" or too
complex. Stay tuned :-)
>> * 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....)
>>  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)