List:Falcon Storage Engine« Previous MessageNext Message »
From:Olav Sandstaa Date:December 4 2008 9:47pm
Subject:Re: Dependency manager and Transaction::commitRecords()
View as plain text  
Olav Sandstaa wrote:
> Kevin Lewis wrote:
>> Olav,  Important point below...
>>
>>
>> Olav Sandstaa wrote:
>>>
>>>
>>> * 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
>
> 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 
>> 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.
>
> 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 just committed this patch. It moves the call to 
TransactionManager::purgeTransactions() from being called by the 
scavenger to being called by Transaction::commit(). So instead of the 
scavenger doing "batch" processing of this job it is now done on every 
commit (soon: only by non-read-only transactions).

Still, I am open for input on what people think is the best solution:
>
> 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()

A "bonus advantage" of this approach is that this is done only by a 
single code path, not five different code paths as with the old 
dependency manager.

I have done a simple performance test and do not see more than what can 
be expected of measurement variation between the approach of 
batchprocessing up to 300.000 transaction objects by the scavenger 
compared to do it this in every commit. The the two lower lines shows 
the performance of alternative 1 and 3 above for 1 to 50 clients doing a 
simple select of a "random record" from a table.

Olav





>>
>>
>>
>>>
>>> * 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)
>>>
>>>
>>>
>
>


Thread
Dependency manager and Transaction::commitRecords()Olav Sandstaa3 Dec
  • Re: Dependency manager and Transaction::commitRecords()Kevin Lewis4 Dec
    • Re: Dependency manager and Transaction::commitRecords()Olav Sandstaa4 Dec
      • Re: Dependency manager and Transaction::commitRecords()Olav Sandstaa4 Dec
        • Re: Dependency manager and Transaction::commitRecords()Kevin Lewis4 Dec
          • Re: Dependency manager and Transaction::commitRecords()Olav Sandstaa5 Dec
            • Re: Dependency manager and Transaction::commitRecords()Olav Sandstaa8 Dec