Skip to content

stdenv/problems: only run problem if it's not ignored#519600

Merged
adisbladis merged 7 commits into
NixOS:masterfrom
llakala:dont-run-all-problems
Jun 14, 2026
Merged

stdenv/problems: only run problem if it's not ignored#519600
adisbladis merged 7 commits into
NixOS:masterfrom
llakala:dont-run-all-problems

Conversation

@llakala

@llakala llakala commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The problems system was designed with an oversight - this bit of code:

  all (
    problem:
    problem.condition attrs -> handlerForProblem pname problem.kindName problem.kindName == "ignore"
  ) automaticProblemsConfigCache

calls problem.condition on every problem, before checking if the problem is ignored. This is specifically meaningful in a nixpkgs context, because maintainerless is not on by default, but it is in automaticProblems. This makes every package check that it has nonempty maintainers when it doesn't have to. What's more, packages with empty maintainers have to call handlerForProblem unnecessarily, which allocates memory, calls some primops, and requires several function calls.

I originally just tried swapping the order of the conditions - but this made things a lot worse, since it now ran handlerForProblem (with its own overhead) on non-broken packages. Instead, I filter the problems to only those that that are defined and non-ignored. This checks both problems.matchers and problems.config, while being a constant-time operation only done once.

In the future, I hope to make handlerForProblem less punishing. As it stands, the function performs a lot of unnecessary logic on every single derivation, when it has all of the data ahead of time, and could do much less.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

@llakala llakala force-pushed the dont-run-all-problems branch from b084faf to 6efe36a Compare May 13, 2026 01:00
nixpkgs-branch-check[bot]

This comment was marked as outdated.

Sign in to view

@nixpkgs-ci nixpkgs-ci Bot requested a review from infinisil May 13, 2026 01:06
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. 6.topic: stdenv Standard environment labels May 13, 2026
@github-project-automation github-project-automation Bot moved this to In Progress in Stdenv May 13, 2026
@llakala llakala force-pushed the dont-run-all-problems branch from 6efe36a to fbf56c5 Compare May 13, 2026 01:31
@llakala llakala marked this pull request as draft May 13, 2026 01:31
@llakala llakala force-pushed the dont-run-all-problems branch from fbf56c5 to 5964033 Compare May 13, 2026 02:00
@nixpkgs-branch-check nixpkgs-branch-check Bot dismissed their stale review May 13, 2026 02:04

Review dismissed automatically

@nixpkgs-ci nixpkgs-ci 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. and removed 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. labels May 13, 2026
@llakala llakala force-pushed the dont-run-all-problems branch 3 times, most recently from c2883e1 to b5921c0 Compare May 13, 2026 03:55
nixpkgs-branch-check[bot]

This comment was marked as outdated.

Sign in to view

@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 2501-5000 This PR causes many rebuilds on Darwin and should target the staging branches. and removed 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 May 13, 2026
@nixpkgs-ci nixpkgs-ci Bot added the 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. label May 13, 2026
@infinisil

infinisil commented May 13, 2026

Copy link
Copy Markdown
Member

Huh, interesting! Can you add a new test for this? See the top of problems.nix for how to run them

@llakala llakala force-pushed the dont-run-all-problems branch from 3219e64 to 317cb76 Compare May 13, 2026 05:31
@llakala llakala marked this pull request as ready for review May 13, 2026 05:31
@llakala

llakala commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Done. Also edited the PR description to clarify the reasoning and logic.

@llakala llakala force-pushed the dont-run-all-problems branch 3 times, most recently from 55dc877 to 52b07e5 Compare May 14, 2026 19:14
@llakala llakala force-pushed the dont-run-all-problems branch from d52b393 to 3724f3d Compare May 25, 2026 01:50
llakala added 7 commits May 25, 2026 23:59
@llakala
stdenv/problems: only run problem if it's not ignored
5c89ab8
@llakala
stdenv/problems: avoid creating manual problems attrset
b88fba2
@llakala
stdenv/problems: use early knowledge of allowBroken and allowBrokenPr…
5616e3d 
…edicate
@llakala
stdenv/problems: avoid a lookup for meta.broken
753e43c 
The stats may be lying to me when they say that this saves 1.86% of
attrset lookups, and `?` may just not be accurately tracked. But at
worst, performance will stay the same, since the check will fail for all
non-broken packages. And who knows, maybe it does help, by nature of not
checking the value!
@llakala
stdenv/problems: reverse parameter order for handlerForProblem
41bafcb 
kind and name are more likely to stay the same. Doing this allows us to
cache the kind and name when calling the handler for a problem.
@llakala
stdenv/problems: make handlerForProblem more performant with partial …
0b30474 
…application
@llakala
stdenv/problems: don't create pname variable
85e32af 
A package failing an automatic problem (of which there's currently only
one) is rare. A package having meta.problems specified is even rarer.
Inlining the getName call to its two usages saves a thunk in the vast,
vast majority of cases, and I consider it to be worth it.
@llakala llakala force-pushed the dont-run-all-problems branch from abad434 to 85e32af Compare June 2, 2026 03:06
@llakala llakala mentioned this pull request Jun 12, 2026
Draft
12 tasks
@adisbladis adisbladis added this pull request to the merge queue Jun 14, 2026
Merged via the queue into NixOS:master with commit 0e62149 Jun 14, 2026
29 of 31 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Stdenv Jun 14, 2026
@llakala llakala deleted the dont-run-all-problems branch June 14, 2026 16:46
infinisil
infinisil reviewed Jun 16, 2026
View reviewed changes

@infinisil infinisil left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit late with the review, but this is great! I tried to get the performance impact as low as possible in the PR that implemented meta.problems (#338267) compared to the original (#177272), but the optimisation here didn't occur to me, so thank you!

I checked out the code as well, no complaints to be found there :)

@llakala llakala mentioned this pull request Jun 24, 2026
Open
13 tasks
@MattSturgeon MattSturgeon added the backport release-26.05 Backport PR automatically label Jun 26, 2026
@nixpkgs-ci

nixpkgs-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Successfully created backport PR for release-26.05:

@nixpkgs-ci nixpkgs-ci Bot mentioned this pull request Jun 26, 2026
Merged
@github-actions github-actions Bot added the 8.has: port to stable This PR already has a backport to the stable release. label Jun 26, 2026
@MattSturgeon MattSturgeon mentioned this pull request Jun 26, 2026
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@infinisil infinisil infinisil left review comments
@nixpkgs-branch-check nixpkgs-branch-check[bot] nixpkgs-branch-check[bot] left review comments

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 8.has: port to stable This PR already has a backport to the stable release. 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. backport release-26.05 Backport PR automatically

Projects

Stdenv
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@llakala @infinisil @adisbladis @MattSturgeon