On 28.6.2007 20:04, Stefan Küng wrote:
> Tomas Kopal wrote:
>> On 28.6.2007 12:23, Jan Rysavy wrote:
>>> Hi Stefan and Tomas,
>>>
>>>> > - Change the global variables to be member variables of the COM
>>> object,
>>>> > thus making them per-instance.
>>>>
>>>> This one is clearly to be prefered. g_filepath, for instance, is
>>>> only used in IsMemberOf (I simply told VS to grep all files). Thus,
>>>> it can safely be made a member variable. My impression is that this
>>>> is true for most globals.
>>> I think that if you store g_filepath to each COM object, it will
>>> lose its "caching" sense (other COM objects called for the same
>>> filename need to use it).
>>>
>>> So I think that storing g_filepath (and others) to TLS is the best
>>> solution to this problem. Or you can leave it global, but you must
>>> access to these variables only in critical section.
>> Hmm, after discussing this a bit further, I have to agree that TLS is
>> probably the best solution here, at least for g_filepath. I will try to
>> describe it a bit more how I understand it:
>>
>> Every icon kind has it's own handler, i.e. it's own COM registered in
>> the registry. In TSVN case, which has about eight different icons
>> depending on the file status, all eight handlers in registry points to
>> the same COM object, which returns that the icon belongs to it based on
>> file status.
>> That means, that for a single file, there will be about eight calls in
>> succession. That's what the g_filepath and other cache global variables
>> are there most probably for - to avoid doing the same processing eight
>> times in a row.
>> All these calls will be done from one thread (remember, it's about
>> finding one icon for one file). But all these calls will be on different
>> instances of the COM interface (each corresponding to a different file
>> state/icon).
>>
>> So, if we change the globals to instance member variables, we loose all
>> the caching potential here. Leaving them global and protecting with e.g.
>> critical section is better in case of access from one thread, but worse
>> if multiple threads access TSVN in the same time.
>>
>> TLS means that the variable will be one per thread, i.e. the caching
>> will work correctly for every thread without being disturbed by other
>> threads running in parallel.
>>
>> So, what do you think now?
>
> You're right. The reason for those global variables is to reduce disk
> access. Because of the many overlay handlers (one for each overlay
> icon), those global objects really improve the performance.
> One problem however with TLS is that we don't have all 'simple' data
> types but also complex objects.
> I think the best way to go here is to use critical sections to guard the
> global objects.
It is not necessary (and possible) to hold complex object directly in TLS.
Pointers to such structures is all we need to store in TLS. So we should
put all object into some allocated structure and store pointer to TLS slot.
Access to TLS slot is FAST.
Global objects guarded with critical sections doesn't look good.
Imagine two threads reading overlays (for different files) at the
same time. Shared global variables (for example g_filepath) will
be frequently invalidated and whole caching mechanism will be violated.
Stefan, thank you for your patch, we will look if it solves all
TortoiseSVN related crashes that our users experiencing.
Best regards
Jan Rysavy
ALTAP
Czech Republic
http://www.altap.cz/
mailto:jan.rysavy@altap.cz
Tel.: +420 487 725 132
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tortoisesvn.tigris.org
For additional commands, e-mail: dev-help@tortoisesvn.tigris.org
Received on Thu Jun 28 20:57:10 2007