)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1000001,"name":"tazjin","email":"tazjin@tvl.su","username":"tazjin"},"change_message_id":"33b6ca55c3869b31271a838482a2c71c137f66e1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"58deb9d9_926ecb2a","updated":"2023-12-12 09:09:11.000000000","message":"Approving; the choice of where to run this is - imo - separate from the benchmark code. I think I agree with neither of you: I\u0027d want a derivation to *build* the benchmark, but *run* it outside :)","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"}],"tvix/glue/benches/eval.rs":[{"author":{"_account_id":1000010,"name":"aspen","email":"root@gws.fyi","username":"aspen"},"change_message_id":"65348b97eb94416b7a9f9e32bf35d165a014320a","unresolved":true,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"bbb482c2_1295f9ff","line":37,"updated":"2023-12-11 04:01:27.000000000","message":"I would much rather pin the nixpkgs this uses, so that changing NIX_PATH in the environment can\u0027t muck with the benchmark results","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000066,"name":"Adam Joseph","display_name":"amjoseph","email":"adam@westernsemico.com","username":"amjoseph"},"change_message_id":"9927ec6eccdb2420b0f44628d86f0a298c11bee5","unresolved":true,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"b6c36ea1_11987880","line":37,"in_reply_to":"85293c0b_da1116a1","updated":"2023-12-12 05:57:51.000000000","message":"Running it inside a derivation is incredibly useful: it\u0027s *reproducible*.\n\nIf it\u0027s so easy to do this in a derivation, please include an example of that.","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000066,"name":"Adam Joseph","display_name":"amjoseph","email":"adam@westernsemico.com","username":"amjoseph"},"change_message_id":"45fb96eeec834b29ddba64f101f2a2118c983a10","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"f5c8363f_afc728dd","line":37,"in_reply_to":"9707bbb4_afcc352d","updated":"2023-12-11 13:10:55.000000000","message":"I don\u0027t see why any of this prevents running the benchmark inside of a derivation.\n\nI\u0027ve been running the macrobenchmarks (cl/10216) in derivations and it works just fine.  Plus then you get a drvPath for the benchmark you just ran!  Very nice for reproducibility.","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"117592c8b4834e396d7d9f04e6b55a3873422923","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"ed2ee5fa_ba1e392f","line":37,"in_reply_to":"b6c36ea1_11987880","updated":"2023-12-12 08:36:45.000000000","message":"It\u0027s not reproducible - the timing information (the output you care about) is highly machine specific. So you kinda want to have control over *where* the final benchmark is run.\n\nThis CL is an incremental addition for manual `cargo bench` runs, in addition to the benchmarks we already have.\n\nIf you\u0027re after not having to build the benchmark itself all the time, that\u0027s blocked on https://github.com/nix-community/crate2nix/issues/284. Once we have that, this could be a small shell script invoking the result of a derivation. \n\nBuilding this in a derivation that emits the build results in its outputs wouldn\u0027t make sense - you would cache runs from other, more or less powerful machines and the results would not be comparable.","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000066,"name":"Adam Joseph","display_name":"amjoseph","email":"adam@westernsemico.com","username":"amjoseph"},"change_message_id":"e8c0d7d842c8fdc11200abef25e6de02f9ac3ca6","unresolved":true,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"dd278e0f_02387648","line":37,"in_reply_to":"bbb482c2_1295f9ff","updated":"2023-12-11 10:28:17.000000000","message":"Agree.\n\nAlso, running the benchmarks inside of derivations helps eliminate extra degrees of freedom like this...","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"0d1720228ccaf4d3a95022915a017d15fa93de91","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"9707bbb4_afcc352d","line":37,"in_reply_to":"dd278e0f_02387648","updated":"2023-12-11 11:19:49.000000000","message":"Wrapping inside a derivation prevents ambient env vars from leaking in, yes, but there\u0027s valid usecases where I want to run it outside a Nix derivation too.\n\nFor example, when comparing it with the previous `cargo bench` run, to locally reason about the impact of a refactor (see commit message).\n\nI think it using `NIX_PATH` for local benchmarks is in theory okay. They\u0027re not meant to be compare-able with CI results anyways, and once we run it in CI, `NIX_PATH` has to be set to something reproducible anyways.\n\nFor non-local benchmarks over longer periods of time, we should probably not (only) track nixpkgs that depot pins to (so we don\u0027t observe nixpkgs restructurings as eval perf changes).\n\nI changed it to `TVIX_BENCH_NIX_PATH`, set that in //tvix:shell, and point it to the depot nixpkgs, it\u0027s probably a good middle ground, though I don\u0027t think it matters much for the current use case.","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"c13d7002cee98f1b9ed9dde4d0e839105b706683","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        \u0026mut eval,"},{"line_number":35,"context_line":"        // The benchmark requires NIX_PATH to be set, so barf out early,"},{"line_number":36,"context_line":"        // rather than benchmarking tvix returning an error"},{"line_number":37,"context_line":"        \u0026Some(env::var(\"NIX_PATH\").expect(\"NIX_PATH must be set\")),"},{"line_number":38,"context_line":"    );"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":"    eval.io_handle \u003d Box::new(tvix_glue::tvix_io::TvixIO::new("}],"source_content_type":"text/x-rustsrc","patch_set":1,"id":"85293c0b_da1116a1","line":37,"in_reply_to":"f5c8363f_afc728dd","updated":"2023-12-11 14:14:57.000000000","message":"It doesn\u0027t prevent us from running it in a derivation, I just don\u0027t think it\u0027s very useful - anyways, that\u0027s a separate discussion, it\u0027s unrelated to the code introduced here.","commit_id":"f89d0fe4c17a2114ec903fae54d0e93cf29e24d5"},{"author":{"_account_id":1000001,"name":"tazjin","email":"tazjin@tvl.su","username":"tazjin"},"change_message_id":"262f6381b8c2383d6609ce5724ac8d4c64ff3d98","unresolved":true,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"fn interpret(code: \u0026str) {"},{"line_number":26,"context_line":"    // TODO: this is a bit annoying."},{"line_number":27,"context_line":"    // It\u0027d be nice if we could set this up once and then run evaluate() with a"},{"line_number":28,"context_line":"    // piece of code. b/262"},{"line_number":29,"context_line":"    let mut eval \u003d tvix_eval::Evaluation::new_impure(code, None);"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    let known_paths: Rc\u003cRefCell\u003cKnownPaths\u003e\u003e \u003d Default::default();"},{"line_number":32,"context_line":"    add_derivation_builtins(\u0026mut eval, known_paths.clone());"}],"source_content_type":"text/x-rustsrc","patch_set":5,"id":"ce70e317_3a026dcf","line":29,"range":{"start_line":27,"start_character":0,"end_line":29,"end_character":0},"updated":"2023-12-12 09:04:59.000000000","message":"Not sure I understand this. You don\u0027t want to benchmark the entire evaluation, but only some part of it? I think it would become very opaque for benchmarking purposes, as there\u0027d be thunk reuse and all sorts of stuff if an evaluation was kept around.","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"e9c21ff19e90218f9bd4949aa7ada274023564c1","unresolved":false,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"fn interpret(code: \u0026str) {"},{"line_number":26,"context_line":"    // TODO: this is a bit annoying."},{"line_number":27,"context_line":"    // It\u0027d be nice if we could set this up once and then run evaluate() with a"},{"line_number":28,"context_line":"    // piece of code. b/262"},{"line_number":29,"context_line":"    let mut eval \u003d tvix_eval::Evaluation::new_impure(code, None);"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    let known_paths: Rc\u003cRefCell\u003cKnownPaths\u003e\u003e \u003d Default::default();"},{"line_number":32,"context_line":"    add_derivation_builtins(\u0026mut eval, known_paths.clone());"}],"source_content_type":"text/x-rustsrc","patch_set":5,"id":"ff92f1b2_488552e0","line":29,"range":{"start_line":27,"start_character":0,"end_line":29,"end_character":0},"in_reply_to":"b6808414_af06b42b","updated":"2023-12-12 10:27:49.000000000","message":"Marked as done, we can still remove this comment if we realize b/262 makes no sense.","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"c433720430cd1521a535915c264558ab17e1b137","unresolved":true,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"fn interpret(code: \u0026str) {"},{"line_number":26,"context_line":"    // TODO: this is a bit annoying."},{"line_number":27,"context_line":"    // It\u0027d be nice if we could set this up once and then run evaluate() with a"},{"line_number":28,"context_line":"    // piece of code. b/262"},{"line_number":29,"context_line":"    let mut eval \u003d tvix_eval::Evaluation::new_impure(code, None);"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"    let known_paths: Rc\u003cRefCell\u003cKnownPaths\u003e\u003e \u003d Default::default();"},{"line_number":32,"context_line":"    add_derivation_builtins(\u0026mut eval, known_paths.clone());"}],"source_content_type":"text/x-rustsrc","patch_set":5,"id":"b6808414_af06b42b","line":29,"range":{"start_line":27,"start_character":0,"end_line":29,"end_character":0},"in_reply_to":"ce70e317_3a026dcf","updated":"2023-12-12 09:16:21.000000000","message":"I\u0027d like to do all the *configuration* and auxillary component setup (what NIX_PATH is configured, what builtins should be available, what\u0027s doing io_handle) before passing in the code to evaluate, and ideally do as little of this static setup inside the timing loop itself.\n\nWe currently need to pass the code to evaluate very early on, so all code \"configuring the evaluator before it runs\" is a bit unergonomic. b/262 tracks a builder pattern and I felt it makes sense to call it out here.","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"},{"author":{"_account_id":1000066,"name":"Adam Joseph","display_name":"amjoseph","email":"adam@westernsemico.com","username":"amjoseph"},"change_message_id":"9927ec6eccdb2420b0f44628d86f0a298c11bee5","unresolved":true,"context_lines":[{"line_number":55,"context_line":"fn eval_nixpkgs(c: \u0026mut Criterion) {"},{"line_number":56,"context_line":"    c.bench_function(\"hello outpath\", |b| {"},{"line_number":57,"context_line":"        b.iter(|| {"},{"line_number":58,"context_line":"            interpret(black_box(\"(import \u003cnixpkgs\u003e {}).hello.outPath\"));"},{"line_number":59,"context_line":"        })"},{"line_number":60,"context_line":"    });"},{"line_number":61,"context_line":"}"}],"source_content_type":"text/x-rustsrc","patch_set":5,"id":"08a90159_99f1e405","line":58,"updated":"2023-12-12 05:57:51.000000000","message":"The test vectors should not be compiled in; we need to be able to specify them externally, from shell scripts or nix code.","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"},{"author":{"_account_id":1000036,"name":"flokli","email":"flokli@flokli.de","username":"flokli"},"change_message_id":"117592c8b4834e396d7d9f04e6b55a3873422923","unresolved":false,"context_lines":[{"line_number":55,"context_line":"fn eval_nixpkgs(c: \u0026mut Criterion) {"},{"line_number":56,"context_line":"    c.bench_function(\"hello outpath\", |b| {"},{"line_number":57,"context_line":"        b.iter(|| {"},{"line_number":58,"context_line":"            interpret(black_box(\"(import \u003cnixpkgs\u003e {}).hello.outPath\"));"},{"line_number":59,"context_line":"        })"},{"line_number":60,"context_line":"    });"},{"line_number":61,"context_line":"}"}],"source_content_type":"text/x-rustsrc","patch_set":5,"id":"d358888c_b792f16c","line":58,"in_reply_to":"08a90159_99f1e405","updated":"2023-12-12 08:36:45.000000000","message":"No, this is a hand-picked example, and we can add others if they make more sense, by adding the code. If you want to time generic tvix-cli invocations, you can just run the tvix-cli binary and time that.","commit_id":"5d5e53ae9d49e1b4afbad97a3dd5f2cd9b1ee182"}]}