Code review comment for lp:~thomas-voss/unity-mir/oom_adjust

> +struct OomAdj
> +struct OomScoreAdj
> Please don't use the "Adj" abbreviation, it's vague. I'd recommend
> OomAdjuster.
>
> + static int disableOomKillerValue()
> + {
> + return -17;
> + }
> Why not just a const static int disableOomKillerValue = -17; Similarly for the
> other *Value() methods.
>

As static constants are less likely to be hit by static initialization fiascos.

> + static int {min,max}Value()
> I'd prefer a more descriptive name like "{least,most}LikelyToKillValue"
>

Hmmm, I intentionally named them min and max, and introduced two more semantic functions to distinguish between what the kernel oom considers min and max and how it is interpreted, respectively.

> + static QString filePattern()
> + {
> + return QString("/proc/%1/oom_adj");
> + }
> Why this in separate public method? It's only used in applyForPid, and is
> internal detail.

yup, fair point.

>
> + throw std::runtime_error("OomScoreAdj created with invalid value.");
> This exception is not caught anywhere, as we tend to avoid using exceptions in
> our Qt code.
>
> +struct OomScoreAdj
> + static OomScoreAdj leastLikelyToBeKilled()
> + {
> + // We could be way more clever here if we knew the distribution
> + // of oom_score_adj values of all app processes. However, we just
> + // make sure that the process is not ignored by the oom killer for now.
> + return OomScoreAdj(minValue()+200);
> + }
> I'd rather you move this comment beside minValue(), and have minValue=-800 for
> now.
>

As explained before, minValue returns the value as defined in linux/oom.h and there is a significant difference between the min value and our interpretation of least likely to be killed.

> focusCallback=....
> + DLOG("startCallback: Could not oom killer probability for process");
> Copy/paste typo in the DLOG
>
> TaskController::resume...
> + DLOG("TaskController::suspend: Could not oom killer probability for
> process");
> Again, c/p typo
>
>
>
> This is a lot of code to do something relatively simple. While I like the
> clear structuring and documentation, creating an object which just writes a
> set of values to a file, and a separate object for slightly different values
> (but with same meaning) to a different file, is overkill IMO.
>
> A singleton class is enough to do this, which would internally handle both
> proc files and their respective values, and so would remove this complexity
> from TaskManager's code:
>

I think we should stop using an anti-pattern like a Singleton _now_. It leads to tight coupling and is increasingly difficult to test. That being said, I do agree that we can further condense updateOom{Score}Adj.

> + if (!updateOomScoreAdj(pid, OomScoreAdj::leastLikelyToBeKilled()))
> + {
> + if (!updateOomAdj(pid, OomAdj::leastLikelyToBeKilled()))
> + DLOG("startCallback: Could not oom killer probability for process");
> + }
>
> reducing it to just:
>
> if (!OomAdjuster::instance()->set(pid, OomAdjuster::leastLikelyToBeKilled)
> DLOG("startCallback: Could not set OOM killer probability for process");