Re: Threading problem in IconOverlay code

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?

Regards

Tomas