Skip to content

stdenv/check-meta: Fix performance regressions introduced in #294347#300177

Merged
adisbladis merged 2 commits intoNixOS:masterfrom
adisbladis:meta-repository-perf-fixups
Mar 30, 2024
Merged

stdenv/check-meta: Fix performance regressions introduced in #294347#300177
adisbladis merged 2 commits intoNixOS:masterfrom
adisbladis:meta-repository-perf-fixups

Conversation

@adisbladis
Copy link
Member

@adisbladis adisbladis commented Mar 30, 2024 •
edited
Loading

Description of changes

This fixes a few performance related regressions added in #294347

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@adisbladis
stdenv/check-meta: Don't create new environments when computing meta.…
f8b091d 
…repository
@adisbladis
stdenv/check-meta: Inherit remaining lib access into scope
dc49349 
Hopefully this will result in people not adding new `lib.xxx` to check-meta.nix.
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Mar 30, 2024
@adisbladis adisbladis mentioned this pull request Mar 30, 2024
Merged
13 tasks
lolbinarycat
lolbinarycat approved these changes Mar 30, 2024
View reviewed changes
Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faster eval is good, and these changes only increase code complexity by a very small amount.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 30, 2024
@adisbladis
Copy link
Member Author

adisbladis commented Mar 30, 2024

Merging as approved.

@adisbladis adisbladis merged commit bff4c55 into NixOS:master Mar 30, 2024
K900 added a commit that referenced this pull request Mar 30, 2024
@K900
Merge pull request #300247 from K900/revert-294347-etc
42b1ad9 
Revert #294347 (and #300177)
@ghost
Copy link

ghost commented Mar 31, 2024

this change doesn't appear to have any effect. the cpu improvement of -%0.46 seems to be just noise. source: look at the performance improvements of the automated r-ryantm version updates that are basically an eval no-op and this is well in the range of those no-op change.

@adisbladis
Copy link
Member Author

adisbladis commented Mar 31, 2024

There are plenty of performance issues that don't show up at all in ofborg because it evaluates the performance report with meta checking disabled.

Also I'm not just concerned about wall time. I'm also concered about GC pressure, unnecessary memory accesses and the like.

@ghost
Copy link

ghost commented Mar 31, 2024 •
edited by ghost
Loading

i see. i am a little confused as #294347 (comment) asked to look at the performance report and here you're saying the performance report doesn't matter because it doesn't check the meta attributes. so the performance report seems like it isn't applicable at all to changes in check-meta.nix. one would need to be generating a report with meta checking enabled. ok, so how would one generate a performance report while checking the meta attributes?

to perform checks locally, i run https://github.com/NixOS/ofborg?tab=readme-ov-file#running-meta-checks-locally though it is non-obvious to me how to generate a report as is done by the CI. Do you know how to generate the report? was that done on this change?

[edit] found NixOS/nix#10256 which mentions NIX_SHOW_STATS_PATH and NIX_SHOW_STATS

@ghost
Copy link

ghost commented Apr 1, 2024 •
edited by ghost
Loading

comparison of this change (opt), patch reverted (unopt) and unopt revert (before). median of 5 runs using local meta checks (x64-linux only).

name before unopt opt
cpuTime 128.21 133.93 131.94
envs.bytes 6577933176 7090432912 7089095808
envs.elements 345162663 374300258 374201006
envs.number 238539492 256001928 255967985
gc.heapSize 18928824320 19985793024 19985793024
gc.totalBytes 40469879104 43413791152 43407905792
list.bytes 778152264 819291024 819291024
list.concats 17173345 17992679 17992679
list.elements 97269033 102411378 102411378
nrAvoided 275230098 295060652 295026708
nrFunctionCalls 219458953 234732209 234732209
nrLookups 113944117 123621375 123553424
nrOpUpdateValuesCopied 602605428 639235093 639235093
nrOpUpdates 28310008 31468152 31468152
nrPrimOpCalls 121898431 128384775 128384775
nrThunks 337764428 366305855 366172660
sets.bytes 13798853440 14793914880 14793914880
sets.elements 809401006 865106113 865106113
sets.number 53027334 59513567 59513567
sizes.Attr 16 16 16
sizes.Bindings 16 16 16
sizes.Env 16 16 16
sizes.Value 24 24 24
symbols.bytes 2322871 2323547 2323560
symbols.number 167259 167328 167329
values.bytes 10519975920 11346920592 11343725400
values.number 438332330 472788358 472655225

and percentages: unopt % = (unopt - baseline) / baseline * 100. opt % = (opt - baseline) / baseline * 100

name baseline unopt % opt %
cpuTime 128.21 4.46 2.91
envs.bytes 6577933176.0 7.79 7.77
envs.elements 345162663.0 8.44 8.41
envs.number 238539492.0 7.32 7.31
gc.heapSize 18928824320.0 5.58 5.58
gc.totalBytes 40469879104.0 7.27 7.26
list.bytes 778152264.0 5.29 5.29
list.concats 17173345.0 4.77 4.77
list.elements 97269033.0 5.29 5.29
nrAvoided 275230098.0 7.21 7.19
nrFunctionCalls 219458953.0 6.96 6.96
nrLookups 113944117.0 8.49 8.43
nrOpUpdateValuesCopied 602605428.0 6.08 6.08
nrOpUpdates 28310008.0 11.16 11.16
nrPrimOpCalls 121898431.0 5.32 5.32
nrThunks 337764428.0 8.45 8.41
sets.bytes 13798853440.0 7.21 7.21
sets.elements 809401006.0 6.88 6.88
sets.number 53027334.0 12.23 12.23
sizes.Attr 16.0 0.0 0.0
sizes.Bindings 16.0 0.0 0.0
sizes.Env 16.0 0.0 0.0
sizes.Value 24.0 0.0 0.0
symbols.bytes 2322871.0 0.03 0.03
symbols.number 167259.0 0.04 0.04
values.bytes 10519975920.0 7.86 7.83
values.number 438332330.0 7.86 7.83

@ghost
Copy link

ghost commented Apr 1, 2024 •
edited by ghost
Loading

getting rid of the optionalAttrs and making repository an empty string when conditions don't match eliminates the performance hit.

name baseline unopt % opt % no-attrs %
cpuTime 128.21 4.46 2.91 -0.64
envs.bytes 6577933176.0 7.79 7.77 0.18
envs.elements 345162663.0 8.44 8.41 0.16
envs.number 238539492.0 7.32 7.31 0.19
gc.heapSize 18928824320.0 5.58 5.58 0.09
gc.totalBytes 40469879104.0 7.27 7.26 0.21
list.bytes 778152264.0 5.29 5.29 0.08
list.concats 17173345.0 4.77 4.77 0.0
list.elements 97269033.0 5.29 5.29 0.08
nrAvoided 275230098.0 7.21 7.19 0.15
nrFunctionCalls 219458953.0 6.96 6.96 0.19
nrLookups 113944117.0 8.49 8.43 0.37
nrOpUpdateValuesCopied 602605428.0 6.08 6.08 0.12
nrOpUpdates 28310008.0 11.16 11.16 0.53
nrPrimOpCalls 121898431.0 5.32 5.32 0.16
nrThunks 337764428.0 8.45 8.41 0.28
sets.bytes 13798853440.0 7.21 7.21 0.22
sets.elements 809401006.0 6.88 6.88 0.19
sets.number 53027334.0 12.23 12.23 0.63
sizes.Attr 16.0 0.0 0.0 0.0
sizes.Bindings 16.0 0.0 0.0 0.0
sizes.Env 16.0 0.0 0.0 0.0
sizes.Value 24.0 0.0 0.0 0.0
symbols.bytes 2322871.0 0.03 0.03 0.0
symbols.number 167259.0 0.04 0.04 0.0
values.bytes 10519975920.0 7.86 7.83 0.26
values.number 438332330.0 7.86 7.83 0.26

@ghost ghost mentioned this pull request Apr 1, 2024
Closed
13 tasks
@lolbinarycat
Copy link
Contributor

lolbinarycat commented Apr 1, 2024

what about making it default to an empty list instead? i think that more appropriately expresses the semantic of "no value", and should be easier to process.

@ghost
Copy link

ghost commented Apr 1, 2024

what about making it default to an empty list instead? i think that more appropriately expresses the semantic of "no value", and should be easier to process.

about the same

name baseline unopt % opt % no-attrs % no-attrs-list %
cpuTime 128.21 4.46 2.91 -0.64 -0.48
envs.bytes 6577933176.0 7.79 7.77 0.18 0.2
envs.elements 345162663.0 8.44 8.41 0.16 0.18
envs.number 238539492.0 7.32 7.31 0.19 0.22
gc.heapSize 18928824320.0 5.58 5.58 0.09 0.09
gc.totalBytes 40469879104.0 7.27 7.26 0.21 0.22
list.bytes 778152264.0 5.29 5.29 0.08 0.08
list.concats 17173345.0 4.77 4.77 0.0 0.0
list.elements 97269033.0 5.29 5.29 0.08 0.08
nrAvoided 275230098.0 7.21 7.19 0.15 0.15
nrFunctionCalls 219458953.0 6.96 6.96 0.19 0.22
nrLookups 113944117.0 8.49 8.43 0.37 0.37
nrOpUpdateValuesCopied 602605428.0 6.08 6.08 0.12 0.12
nrOpUpdates 28310008.0 11.16 11.16 0.53 0.53
nrPrimOpCalls 121898431.0 5.32 5.32 0.16 0.16
nrThunks 337764428.0 8.45 8.41 0.28 0.3
sets.bytes 13798853440.0 7.21 7.21 0.22 0.22
sets.elements 809401006.0 6.88 6.88 0.19 0.19
sets.number 53027334.0 12.23 12.23 0.63 0.63
sizes.Attr 16.0 0.0 0.0 0.0 0.0
sizes.Bindings 16.0 0.0 0.0 0.0 0.0
sizes.Env 16.0 0.0 0.0 0.0 0.0
sizes.Value 24.0 0.0 0.0 0.0 0.0
symbols.bytes 2322871.0 0.03 0.03 0.0 0.0
symbols.number 167259.0 0.04 0.04 0.0 0.0
values.bytes 10519975920.0 7.86 7.83 0.26 0.28
values.number 438332330.0 7.86 7.83 0.26 0.28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@piegamesde piegamesde Awaiting requested review from piegamesde

1 more reviewer

@lolbinarycat lolbinarycat lolbinarycat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adisbladis @lolbinarycat