-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
check-meta.nix: Add package warnings #338267
base: master
Are you sure you want to change the base?
Conversation
I will rebase then work on solving conflicts, marking this PR as draft for now. |
Run with `nix-build -A tests.problems`
03ea8fe
to
f56564b
Compare
f56564b
to
e9eff04
Compare
e9eff04
to
1589696
Compare
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
a1269fe
to
69b5584
Compare
++ lib.optionals (meta ? problems) ( | ||
|
||
# Check problem kinds are correct | ||
lib.pipe meta.problems [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could be used to have better performance whilst keeping the same or similar functionality ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 = relaxed
sandbox = true
nix-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.