Re: [PATCH] improve hack in src/base/TextException.h

On 02/22/2011 11:29 AM, Kinkie wrote:

> I agree that now that the portability fix is in, there is no real need
> to fox what ain't broken. I like the dummy user class much less than
> the GCC-specific attribute (which may be wrapped better, my sugested
> approach was meant not to introduce any new includes in
> TexhException.h). I have no issues at all with the few extra bytes,
> it's the hack I don't like. Also because it ends up being nonportable
> at all: someday some compiler will implement cleverer unuysed
> detection and we'll be nback to square 1.

The __attribute__ hack is not a portable solution because you will need
similar hacks for every compiler that complains about unused statics.

The added user class is a portable solution, AFAICT. No compiler can
declare that class unused (without declaring a lot of other Squid code
unused) because the class is global in scope and not limited to a single
compilation unit or library. The class had a minor implementation bug
that icc detected, but that is irrelevant to this discussion.

> I agree that having object-file-specific informations (similar to cvs
> $Id$, I suppose) is useful. I wish bzr supported that, it's not news..

I am not sure we should rely on VCS for these IDs because not all source
files are under VCS control. A more general solution is needed (but
VCS-generated IDs might be a part of it).

> Anyways: let's not change again what has been already fixed. We can
> resume this thread if and when some compiler will try to outsmart us.

Sounds good to me.

Thank you,

Alex.

> On 2/22/11, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
>> On 02/22/2011 08:36 AM, Tsantilas Christos wrote:
>>> On 02/22/2011 04:47 PM, Kinkie wrote:
>>>> because it involves tricks to embed the same code again and again in
>>>> different compilation units to perform the caching.
>>
>>> Well, I do not have strong opinion on that, we can remove caching if
>>> we do not like. But this is a problem which it is solved and I doubt
>>> that it can cause more problems. If we believe that every time we are
>>> going to add a new module or feature we have to spend time for a new
>>> solution/hack to fix it, then we should remove it. This is just my
>>> opinion.
>>
>> I may have been the one who suggested this optimization, but I am not
>> strongly attached to it, and I am not against removing it if it causes
>> more problems than it solves.
>>
>> I do not think adding a few bytes to every .o file to store the computed
>> hash value is a problem, but others may find bigger flaws with the
>> approach, of course.
>>
>> Perhaps others can vote and break the current tie? Current code caches
>> the source code file name hash to avoid recomputing it for every failed
>> Must(). Must()s do not fail often, but, unlike assert()s, they may fail
>> when there are I/O and compatibility failures. The cost of such caching
>> is a few extra bytes added to every .o file that uses TextException. The
>> cost of not caching is computing a simple hash for a 10-40 byte string
>> (depending on the source file and the compiler).
>>
>>>> IMVHO it is not
>>>> even worth the time we have collectively spent in this discussion ;)
>>
>> That is probably true, at least short-term. FWIW, I would not bother
>> keeping this pandora box open if it were not for the GCC-specific
>> __attribute__ that was added by those who opened it :-).
>>
>> Thank you,
>>
>> Alex.
>> P.S. Eventually, I think every source file may get a unique name/ID that
>> does not depend on the compiler, so this code will have to be changed
>> again, but we are not there yet.
>>
>>
>>
>>>> On 2/22/11, Tsantilas Christos<chtsanti_at_users.sourceforge.net> wrote:
>>>>> On 02/22/2011 03:18 PM, Tsantilas Christos wrote:
>>>>>> On 02/22/2011 02:08 PM, Kinkie wrote:
>>>>>>> Hi Christos,
>>>>>>> My question is, why use the cache at all? It seems quite complex for
>>>>>>> a little gain..
>>>>>>
>>>>>> You mean the FileNameHashCached method?
>>>>>> It is used to compute the hash of the file name to be used inside the
>>>>>> "Must" function. We may have multiple Must calls inside a simple
>>>>>> function. We are calling many times the "Must" function for each HTTP
>>>>>> requests comes through squid.
>>>>>> If we do not cache the result we are going to compute this hash on
>>>>>> every
>>>>>> Must call. It is not normal.
>>>>>>
>>>>>> The FileNameHashCached is not complex, it is a very simple function.
>>>>>> And
>>>>>> the gain is huge not little.
>>>>>
>>>>> Well, with a second view, I saw that the FileNameHashCached called only
>>>>> if the argument/condition passed to Must is not true (on failures etc).
>>>>> So yes it is not important. We are expecting to have a small number of
>>>>> such cases.
>>>>> Anyway I do not think it is so complex this part of code (just an if),
>>>>> why should we compute this value again and again?
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> kinkie
>>>>>>> roaming
>>>>>>>
>>>>>>> Il giorno 22/feb/2011 11.54, "Tsantilas Christos"
>>>>>>> <chtsanti_at_users.sourceforge.net<mailto:chtsanti_at_users.sourceforge.net>>
>>>>>>>
>>>>>>> ha scritto:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> On 02/21/2011 08:31 PM, Alex Rousskov wrote:
>>>>>>>>> On 02/21/2011 12:27 AM, Kinkie wrote:
>>>>>>>>>>> Sorry, I am not sure I understand the above. Should I reverse
>>>>>>> r11236 and
>>>>>>>>>>> commit a casting fix? Or do you want be to post a patch that some
>>>>>>> Hudson
>>>>>>>>>>> job will pick up and test somehow first?
>>>>>>>>>>
>>>>>>>>>> Why?
>>>>>>>>>> Judging from the comment in there it's just meant to trick the
>>>>>>>>>> compiler into not (mis)understanding that the FileNameHash
>>>>>>> function is
>>>>>>>>>> unused, because in the end it's invoked via trickery.
>>>>>>>>>
>>>>>>>>> FileNameHash is _sometimes_ unused. The function is used in
>>>>>>> compilation
>>>>>>>>> units where certain macros are used and only in those compilation
>>>>>>> units.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> IMVHO there are two clean ways out of this:
>>>>>>>>>> - use a compiler-dependent thing such as __attribute__((unused)) to
>>>>>>>>>> politely inform the compiler that sometimes the programmer knows
>>>>>>>>>> more
>>>>>>>>>> about stuff than the compiler
>>>>>>>>>
>>>>>>>>> A compiler-specific attribute is not a "clean way", especially
>>>>>>>>> when it
>>>>>>>>> is introduced as a fix for a rather different problem and actually
>>>>>>> kind
>>>>>>>>> of lies to the compiler in some cases.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> - just get rid of this whole FileNameHashCached thing as a
>>>>>>>>>> premature
>>>>>>>>>> optimization, and just recalculate the filename hash when we
>>>>>>>>>> throw a
>>>>>>>>>> TextException (which is hopefully not that often)
>>>>>>>>>
>>>>>>>>> Sure, let's revisit the necessity of this hash caching optimization.
>>>>>>>>>
>>>>>>>>> Christos, do you remember what were the key reasons for adding it
>>>>>>> in the
>>>>>>>>> first place?
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, with a little research I found it :-)
>>>>>>>> This is a part of the log error details patch.
>>>>>>>>
>>>>>>>> The original comment was:
>>>>>>>>> ... For the files the Must is not used, I got a compiler error that
>>>>>>> " the FileNameHashCached defined but not used"
>>>>>>>>> To solve it I just define a foo_class....
>>>>>>>>
>>>>>>>>
>>>>>>>> I am not sure about the "__attribute__((unused))" definition but
>>>>>>>> if it
>>>>>>>> is only suppressing the warning, it is not bad idea...
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>>
>>>>>>>>> Alex.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>
>>
>
>
Received on Tue Feb 22 2011 - 18:51:41 MST