Vladislav Vaintroub wrote:
>>> -In SerialLog::createNewWindow you have an optimization to only
>>>
>> read
>>
>>> the size of the file if the highWater == 0. This saves a system call
>>> once in a while, but if I understand the code correctly this code only
>>> runs each time you actually switches to a new log file. If this is the
>>> case, I am not sure if an extra system call is worth to optimize away?
>>>
>> Disagree. (highWater == 0) means that you are actually going to
>> switch
>> files. If it is not zero, which is most of the time by far, you don't
>> want
>> to do this stuff.
>>
>>
> Olav's original point (and I agreed with this point already) is that using
> highWater and saving fstat/GetFileSizeEx
> does not seem to bring a measurable performance gain. On the other hand,
> messing up with highWater makes code harder to read.
>
In addition to making it harder to read it also makes it harder to
maintain since it would introduce a new dependency on highWater. If
anyone should need to change some code that maintain the value of
highWater they had to take into account this new usage of it.
About the performance impact of having this system call. If I understood
the code correctly this will be called once for each log file switch.
With a default size of log file of 10 MB and with current disks able to
write maximum (optimistically) 100 MB/s this should result in maximum 10
extra system calls per second (much less than the context switch frequency).
> After thinking a while about it, I prefer to check in a version that *does
> not* use highWater hack. But if measurements done by PAE, Kelly, Intel or
> Hakan show a bottleneck here, we can use this optimization. In my opinion
> it could be a sane overall strategy for Falcon (don't optimize too early and
> try to keep code straight, unless measurement shows potential savings of a
> particular optimization).
>
Mostly agree, I still think it is important to look for optimizations
early but then only include them if we think they might have an impact.
>>> -For the two new member functions in the SerialLogFile class you
>>>
>> use
>>
>>> uint64 for representing the size of the file. The exiting member
>>> functions and internal variables use int64 for representing the file
>>>
>> size.
>>
>> Agree to use signed.
>>
>
> So , now there are 3 of us who believe in existence of negative file sizes;)
>
:-) It is more that I believe in consistent interfaces. I would not
object if we rewrote all the methods of the SerialLogFile class to use
unsigned types for values relating to the file size.
..olav
