check-meta.nix: Add package warnings #338267
Conversation
github-actions
bot
added
8.has: documentation
6.topic: stdenv
|
I will rebase then work on solving conflicts, marking this PR as draft for now. |
ofborg
bot
added
the
2.status: merge conflict
Run with `nix-build -A tests.problems`
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
3 times, most recently
from
03ea8fe
to
f56564b
Compare
AkechiShiro
removed
the
2.status: merge conflict
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
from
f56564b
to
e9eff04
Compare
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
from
e9eff04
to
1589696
Compare
ofborg
bot
added
8.has: clean-up
10.rebuild-darwin: 0
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
2 times, most recently
from
2ce1321
to
a1269fe
Compare
* Also fix merge conflicts * Make sure that performance PR is conserved * Use RunCommand instead of alias RunCommandNoCC * Add enum types to meta-types.nix and use them for problemTypes * Nixfmt (excepted check-meta.nix) * "import" elem * Remove dead code
AkechiShiro
force-pushed
the
rfc127-implementation-test
branch
from
a1269fe
to
69b5584
Compare
| ++ lib.optionals (meta ? problems) ( | ||
|
|
||
| # Check problem kinds are correct | ||
| lib.pipe meta.problems [ |
There was a problem hiding this comment.
This all looks very allocation/iteration heavy:
I'd completely avoid using lib.pipe in any hot paths for one.
For a normal code review I wouldn't be so pedantic about this, but check-meta is a performance special case.
Changes here needs to be carefully measured, and the Ofborg performance report doesn't include checkMeta, so it's entirely useless in this context.
Measuring nixpkgs eval with checkMeta = true before and after this PR (env NIX_SHOW_STATS=1 nix-env -qa -f ./outpaths.nix --arg checkMeta true > /dev/null):
- Before
{
"cpuTime": 271.2768249511719,
"envs": {
"bytes": 7850426888,
"elements": 413454253,
"number": 283924554
},
"gc": {
"heapSize": 22804639744,
"totalBytes": 47139879824
},
"list": {
"bytes": 987373624,
"concats": 21648163,
"elements": 123421703
},
"nrAvoided": 329081776,
"nrFunctionCalls": 261979419,
"nrLookups": 131336801,
"nrOpUpdateValuesCopied": 675865734,
"nrOpUpdates": 32046977,
"nrPrimOpCalls": 146549876,
"nrThunks": 399878601,
"sets": {
"bytes": 15552994336,
"elements": 911935337,
"number": 60126809
},
"sizes": {
"Attr": 16,
"Bindings": 16,
"Env": 16,
"Value": 24
},
"symbols": {
"bytes": 2887399,
"number": 178950
},
"values": {
"bytes": 12350285592,
"number": 514595233
}
}- After
{
"cpuTime": 296.9483947753906,
"envs": {
"bytes": 8506454064,
"elements": 438912624,
"number": 312197067
},
"gc": {
"heapSize": 22737526784,
"totalBytes": 48902439376
},
"list": {
"bytes": 1000276576,
"concats": 22264431,
"elements": 125034572
},
"nrAvoided": 349063893,
"nrFunctionCalls": 289412278,
"nrLookups": 149742381,
"nrOpUpdateValuesCopied": 694111348,
"nrOpUpdates": 35108342,
"nrPrimOpCalls": 168992563,
"nrThunks": 417292743,
"sets": {
"bytes": 15871244064,
"elements": 927562812,
"number": 64389942
},
"sizes": {
"Attr": 16,
"Bindings": 16,
"Env": 16,
"Value": 24
},
"symbols": {
"bytes": 4092168,
"number": 234982
},
"values": {
"bytes": 12774278736,
"number": 532261614
}
}There was a problem hiding this comment.
What could be used to have better performance whilst keeping the same or similar functionality ?
There was a problem hiding this comment.
-
Don't keep calling mapAttrsToList
mapAttrsToList isattrs: map fn (attrNames attrs).
You should store attrNames in a variable and keep reusing that -
lib.remove null -> concatMap
Nix has an optimisation for empty lists
The code below is a good simple example:
# Check that problem has a message (required field)
++ lib.pipe meta.problems [
(lib.mapAttrsToList ( name: problem:
if problem ? message then
null
else
"key 'meta.problems.${name}.message' is missing"
))
(lib.remove null)
]->
let
# Stick this in the outermost scope where it makes sense
problemNames = attrNames meta.problems;
in
# Nix has an optimisation for returning empty lists
# And another optimisation for lists with <= 2 elements
concatMap (name: if meta.problems.${name} ? message then [] else [ "key 'meta.problems.${name}.message' is missing" ])There was a problem hiding this comment.
And just in general: Read the lib implementations you're using here and understand that Nix is not very optimised.
Sadly we don't have good guidance on how to write performant Nix code.
NIX_SHOW_STATS=1 is your friend.
| # Check that some problem kinds are unqiue | ||
| ++ lib.pipe meta.problems [ | ||
| (lib.mapAttrs (name: problem: { kind = name; } // problem)) | ||
| # Count the number of instances of each problem kind, |
There was a problem hiding this comment.
I think the manual counting & checking is better implemented by combining groupBy & concatMap over problemKindsUnique'.
Description of changes
Implements RFC 127 NixOS/rfcs#127
Adds testing for multiple cases
Should supersede : #177272
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.