[systemd-devel] [PATCH] bootchart: use conf-parser & CamelCase names in .conf

[Kok, Auke-jan H]

On Thu, Feb 14, 2013 at 3:07 AM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
> On Thu, Feb 14, 2013 at 1:37 AM, Kok, Auke-jan H
> <auke-jan.h.kok at intel.com> wrote:
>> On Wed, Feb 13, 2013 at 4:00 PM, Kay Sievers <kay at vrfy.org> wrote:
>>> On Thu, Feb 14, 2013 at 12:42 AM, Kok, Auke-jan H
>>> <auke-jan.h.kok at intel.com> wrote:
>>>> On Wed, Feb 13, 2013 at 3:32 PM, Lennart Poettering
>>>> <lennart at poettering.net> wrote:
>>>>> On Wed, 13.02.13 14:27, Kok, Auke-jan H (auke-jan.h.kok at intel.com) wrote:
>>>>>
>>>>>> > Hmm, what does this stand for? Wikipedia doesn't have it, can't be that
>>>>>> > well known...
>>>>>>
>>>>>> PSS is the alternative to RSS... You probably won't find an
>>>>>> explanation anywhere else but the kernel source code:
>>>>>>
>>>>>> Documentation/filesystems/proc.txt:
>>>>>> =====
>>>>>> The /proc/PID/smaps is an extension based on maps, showing the memory
>>>>>> consumption for each of the process's mappings. For each of mappings there
>>>>>> is a series of lines such as the following:
>>>>>>
>>>>>> 08048000-080bc000 r-xp 00000000 03:02 13130      /bin/bash
>>>>>> Size:               1084 kB
>>>>>> Rss:                 892 kB
>>>>>> Pss:                 374 kB
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>> The first of these lines shows the same information as is displayed for the
>>>>>> mapping in /proc/PID/maps.  The remaining lines show the size of the mapping
>>>>>> (size), the amount of the mapping that is currently resident in RAM (RSS), the
>>>>>> process' proportional share of this mapping (PSS),
>>>>>> =====
>>>>>>
>>>>>> so, PSS translates to "proportional share of the mapping(size) that is
>>>>>> resident in RAM"
>>>>>>
>>>>>> PSS will do fine, I suppose :^)
>>>>>
>>>>> RSS is an acronym for "Residential Set Size". PSS for
>>>>> "Propertional Set Size". Hence the option for bootchart should be
>>>>> "ProportionalSetSize="?
>>>>>
>>>>> What does the option actually do? Do we actually need the option? If
>>>>> not, we might just drop this source of confusion? And we do need it,
>>>>> maybe make it explanatory as int "PlotProportionalSetSize=" or so?
>>>>
>>>> When enabled, it creates an additional graph (just like the entropy
>>>> option, or, if you have booted with initcall_debug) that plots the PSS
>>>> for each process.
>>>>
>>>> It's a highly usable graph for people working on systems with less
>>>> memory, so, I'd like to keep it.
>>>>
>>>> Example of how it looks here:
>>>>
>>>>     http://foo-projects.org/~sofar/bootchart-20120401-0710.svg
>>>>
>>>> Plotting of PSS is disabled by default since it has quite a
>>>> performance impact (it requires parsing /proc/<NN>/smaps for each
>>>> process, which can be hundreds of kilobytes large each).
>>>
>>> That looks nice, yeah.
>>>
>>> But shouldn't it just be called PlotMemoryUsage= or something instead
>>> of the using the "algorithm name" in the config switch to enable it?
>>
>> Right, that's totally fine with me, really. I suck at naming things ;^)
>>
>> EntropyGraph... similarly thus.
>
> So, can I commit the patch tonight with these names:
>
> Rel => Relative
> Freq => Frequency
> Entropy => PlotEntropyGraph
> Pss => PlotMemoryUsage
>
> and parsing these as bool so the .conf file will have e.g.
> "PlotMemoryUsage=no" ?

absolutely, yes.

> What about the options from the cli? Should we keep them as e.g.
> "--freq" for backwards compatibility or make similar changes there?

I suggest tackling that in a different patch... I'm not very happy
with the current option naming myself - I've considered changing the
runtime model to one where we specify a length in time (seconds)
instead of specifying a length in samples, which simplifies things a
bit. That should also go together with a patch where we remove all the
static arrays so we can run for arbitrary run lengths...

Auke