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

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 - 14:01:25 MST