optimize derivation parsing #9673

pennae · 2023-12-28T17:54:29Z

Motivation

there's a lot of performance to be had in raw derivation handling. depending on the workload and compile options we see anywhere from 5 to 10% improvement for drv-creation-heavy workloads.

Context

all benchmarks here are done on an empty store for each run, thus recreating all derivations from scratch each time. tests have been compiled with lto because nixpkgs does the same.

if the drvs already exist we don't see nearly as much benefit, if any at all.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

roberth

A few superficial suggestions. LGTM content-wise.

roberth · 2023-12-28T21:17:44Z

src/libstore/derivations.cc

+/* This mimics std::istream to some extent. We use this much smaller implementation
+   instead of plain istreams because the sentry object overhead is too high. */


Suggested change

/* This mimics std::istream to some extent. We use this much smaller implementation

instead of plain istreams because the sentry object overhead is too high. */

/**

* This mimics std::istream to some extent. We use this much smaller implementation

* instead of plain istreams because the sentry object overhead is too high.

*/

roberth · 2023-12-28T21:19:20Z

src/libstore/derivations.cc

+namespace {
+/* This mimics std::istream to some extent. We use this much smaller implementation
+   instead of plain istreams because the sentry object overhead is too high. */
+struct ParseContext {


Suggested change

struct ParseContext {

struct StringViewStream {

Covers the meaning.

roberth · 2023-12-29T21:11:25Z

src/libstore/derivations.cc

+/* This mimics std::istream to some extent. We use this much smaller implementation
+   instead of plain istreams because the sentry object overhead is too high. */
+struct ParseContext {
+    std::string_view str;


str is rather ambiguous. How about either

Suggested change

std::string_view str;

std::string_view remaining;

or

Suggested change

std::string_view str;

std::string_view view;

roberth · 2023-12-29T21:16:00Z

src/libstore/derivations.cc

+
+    std::string res;
+    res.reserve(content.size());
+    for (auto c = content.begin(), end = content.end(); c != end; c++)


This c shadows the other c. Either put the other c in a block, or reuse it (ie no auto declaration, but an assignment).
Same for end.

roberth · 2023-12-29T21:24:57Z

src/libstore/derivations.cc

-    int c;
-    while ((c = str.get()) != '"')
-        if (c == '\\') {
-            c = str.get();


resolved:
Looks like we had a "bug" where we accepted invalid strings and inserted \xFF, but only when parsing a bare ATerm string (ie not in a tuple or anything), which I don't think ever occurred.

this is not theoretical; truncating a .drv in the middle of a string causes nix to OOM

Worth a release note?

not entirely sure. it's not a security issue and we expect drvs truncated like this to not happen in practice, but who knows.

I think it is worth noting. Thanks!

istream sentry objects are very expensive for single-character operations, and since we don't configure exception masks for the istreams used here they don't even do anything. all we need is end-of-string checks and an advancing position in an immutable memory buffer, both of which can be had for much cheaper than istreams allow. the effect of this change is most apparent on empty stores. before: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 7.167 s ± 0.013 s [User: 5.528 s, System: 1.431 s] Range (min … max): 7.147 s … 7.182 s 10 runs after: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.963 s ± 0.011 s [User: 5.330 s, System: 1.421 s] Range (min … max): 6.943 s … 6.974 s 10 runs

more buffers that can be uninitialized and on the stack. small difference, but still worth doing. before: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.963 s ± 0.011 s [User: 5.330 s, System: 1.421 s] Range (min … max): 6.943 s … 6.974 s 10 runs after: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.952 s ± 0.015 s [User: 5.294 s, System: 1.452 s] Range (min … max): 6.926 s … 6.974 s 10 runs

a bunch of derivation strings contain no escape sequences. we can optimize for this fact by first scanning for the end of a derivation string and simply returning the contents unmodified if no escape sequences were found. to make this even more efficient we can also use BackedStringViews to avoid copies, avoiding heap allocations for transient data. before: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.952 s ± 0.015 s [User: 5.294 s, System: 1.452 s] Range (min … max): 6.926 s … 6.974 s 10 runs after: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.907 s ± 0.012 s [User: 5.272 s, System: 1.429 s] Range (min … max): 6.893 s … 6.926 s 10 runs

the table is very small compared to cache sizes and a single indexed load is faster than three comparisons. before: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.907 s ± 0.012 s [User: 5.272 s, System: 1.429 s] Range (min … max): 6.893 s … 6.926 s 10 runs after: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.883 s ± 0.016 s [User: 5.250 s, System: 1.424 s] Range (min … max): 6.860 s … 6.905 s 10 runs

many paths need not be heap-allocated, and derivation env name/valye pairs can be moved into the map. before: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.883 s ± 0.016 s [User: 5.250 s, System: 1.424 s] Range (min … max): 6.860 s … 6.905 s 10 runs after: Benchmark 1: nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' Time (mean ± σ): 6.868 s ± 0.027 s [User: 5.194 s, System: 1.466 s] Range (min … max): 6.828 s … 6.913 s 10 runs

pennae requested review from thufschmitt and edolstra as code owners December 28, 2023 17:54

roberth reviewed Dec 29, 2023

View reviewed changes

pennae added 2 commits December 30, 2023 00:45

pennae force-pushed the drv-parse-opts branch from 475fc07 to 135be74 Compare December 30, 2023 00:35

roberth approved these changes Dec 30, 2023

View reviewed changes

pennae added 3 commits December 30, 2023 18:44

pennae force-pushed the drv-parse-opts branch from 135be74 to c62686a Compare December 30, 2023 17:44

roberth merged commit 3511430 into NixOS:master Dec 31, 2023
8 checks passed

pennae deleted the drv-parse-opts branch December 31, 2023 21:01

fricklerhandwerk added the performance label Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize derivation parsing #9673

optimize derivation parsing #9673

pennae commented Dec 28, 2023

roberth left a comment

roberth Dec 28, 2023

roberth Dec 28, 2023

roberth Dec 29, 2023

roberth Dec 29, 2023

roberth Dec 29, 2023

pennae Dec 30, 2023 •

edited

roberth Dec 30, 2023

pennae Dec 30, 2023

roberth Dec 31, 2023

		/* This mimics std::istream to some extent. We use this much smaller implementation
		instead of plain istreams because the sentry object overhead is too high. */

optimize derivation parsing #9673

optimize derivation parsing #9673

Conversation

pennae commented Dec 28, 2023

Motivation

Context

Priorities and Process

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pennae Dec 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pennae Dec 30, 2023 •

edited