about summary refs log tree commit diff
path: root/third_party/nix/src/libexpr
AgeCommit message (Collapse)AuthorFilesLines
2020-08-14 r/1646 refactor(tvix): completely remove boehm gcKane York11-155/+47
We have decided that leaking memory is a better fate than random, non-debuggable memory corruption. Future CLs will begin changing various fields to std::unique_ptr and std::shared_ptr. It turns out that disabling the GC does not have disasterous impact. The Nix evaluator only runs on the client CLI, never in any long- running process. Even the REPL does not leak too badly under this change, because it uses one EvalState for the duration of the REPL. Building an explicitly tracing garbage collector is likely in the future of this project, but that giant amount of work cannot be done under a nix evaluator that is constantly crashing. We need to restore development velocity here, and this is the best way we've figured out to do it. Change-Id: I2fcda8fcee853c15a9a5e22eca7c5a784bc2bf76 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1720 Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in> Tested-by: BuildkiteCI
2020-08-13 r/1642 refactor(tvix): Very minor lint fixesVincent Ambo2-3/+4
Change-Id: I4aca504d98c79f931b7faadff86f40d27ed5a2c5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1719 Tested-by: BuildkiteCI Reviewed-by: kanepyork <rikingcoding@gmail.com>
2020-08-10 r/1633 fix(3p/nix): inherit Expr from gc, make parser state traceableKane York4-40/+56
The parser contained vectors, and the primary parser state, that were not participating in GC tracing. Change-Id: Ie198592cd7acffd390e3e2ae9595138b56416838 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1706 Tested-by: BuildkiteCI Reviewed-by: tazjin <mail@tazj.in> Reviewed-by: glittershark <grfn@gws.fyi>
2020-08-10 r/1632 style(tvix): Sort out minor formatting errorsVincent Ambo1-3/+3
Invocations of the MakeError macro that were not followed by a semicolon messed up indentation in the next lines. Change-Id: I03d7d1443f062a38af2c7da3da8928e0ed05e274 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1708 Tested-by: BuildkiteCI Reviewed-by: kanepyork <rikingcoding@gmail.com>
2020-08-09 r/1624 fix(tvix): Make nix value structs inherit from GCGriffin Smith1-5/+5
All of the miscellanious structs that make up a nix Value should inherit from gc, as they contain pointers to GC'ed things as members Fixes: #42 Change-Id: I057d8c9f7dafbee7de7644ff152c9cba1aa6bc03 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1696 Tested-by: BuildkiteCI Reviewed-by: kanepyork <rikingcoding@gmail.com>
2020-08-08 r/1619 chore(3p/nix): apply google-readability-castingKane York5-12/+18
Command run: jq <compile_commands.json -r 'map(.file)|.[]' | grep -v '/generated/' | parallel clang-tidy -p compile_commands.json -checks=-*,google-readability-casting --fix Manual fixes applied in src/nix-env/nix-env.cc, src/libstore/store-api.cc Change-Id: I406b4be9368c557ca59329bf6f7002704e955f8d Reviewed-on: https://cl.tvl.fyi/c/depot/+/1557 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in>
2020-08-08 r/1616 chore(3p/nix): don't optional-wrap EvalState::file_access_trace_fn furtheredef2-4/+3
std::function has a natural null we can't eliminate anyway, so this was effectively std::optional<std::optional<non_nullable_function>>. Change-Id: If99f139146021edb25d133dad7f0f6e125ef53df Reviewed-on: https://cl.tvl.fyi/c/depot/+/1688 Tested-by: BuildkiteCI Reviewed-by: kanepyork <rikingcoding@gmail.com> Reviewed-by: glittershark <grfn@gws.fyi>
2020-08-06 r/1613 fix(3p/nix): Use SkipEmpty in all calls to absl::StrSplitKane York3-4/+5
The behavior to return a list containing a single empty string when provided an empty string is a behavior that absl inherited from legacy code. However, the behavior expected by legacy code in Nix is the behavior provided by the SkipEmpty option. Switch all calls to use SkipEmpty, except for the call already using SkipWhitespace. See also commit 26a59482d2427f640893517f1b24dd650a5bd5da, with the partly-prophetic message: "there may be other places we need to fix this as well." Change-Id: I6e94856a12cfb1b7e4a3b4e221769ed446648861 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1687 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-08-06 r/1604 chore(3p): Bump nixpkgs channelsVincent Ambo1-1/+1
Bumps both nixos-unstable and nixos-20.03 to today's versions, as per status.nixos.org Contains minor fixes to things that broke because of the update: * tazjin/frog: hardware.u2f is a deprecated setting * glittershark/system: modSha256 in Go modules is now vendorSha256 * glittershark/owothia: removed version constraint on relude Change-Id: Ib3e9612b1b06ed547b90e4f8b0ffe5ed7fe0a5c4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1642 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-08-05 r/1603 feat(3p/nix): add --trace-file-access to nix-instantiateKane York2-0/+23
This builds on edef's work with depot-scan by adding a dedicated flag to the command. We piggyback on upstream's restricted-mode implementation, the checkSourcePath function. Change-Id: I52bb613549f40dbca1e8caa036635910c1a3d6d0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1654 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-08-03 r/1557 refactor(3p/nix): Only initialise garbage-collector where neededVincent Ambo2-66/+69
Only libexpr depends on the garbage collector, specifically only instantiations of EvalState actually require the GC to be initialised. Rather than always starting it for the whole program, even if it is not needed, this change moves the GC initialisation into libexpr, guarded by absl::call_once. This should make it possible to run the nix daemon without the garbage collector interfering, granted that things are correcty separated and the daemon does not actually invoke the evaluator. Based on my investigation so far, the daemon logic itself does not require libexpr to be present at all - so I think it is safe - but the current monobinary might have some tricks up its sleeve that will cause problems for us. We can deal with those if they arise. Relates to https://b.tvl.fyi/issues/30 Change-Id: I61c745f96420c02e089bd3c362ac3ccb117d3073 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1584 Tested-by: BuildkiteCI Reviewed-by: edef <edef@edef.eu> Reviewed-by: kanepyork <rikingcoding@gmail.com>
2020-08-01 r/1534 fix(3p/nix): revert "apply all clang-tidy fixes"Kane York12-133/+106
This reverts commit ef54f5da9fa30b5c302f2a49595ee5d041f9706a. Resolved conflicts: third_party/nix/src/libexpr/eval.cc third_party/nix/src/libstore/builtins/fetchurl.cc third_party/nix/src/libstore/references.cc third_party/nix/src/libutil/hash.cc third_party/nix/src/nix-daemon/nix-daemon.cc Change-Id: Ib9cf6e96a79a23bde3983579ced3f92e530cb011 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1547 Reviewed-by: glittershark <grfn@gws.fyi> Tested-by: BuildkiteCI
2020-08-01 r/1530 fix(3p/nix/expr): use traceable_allocator for btree_mapKane York1-3/+3
The prior use of gc_allocator meant that the btree nodes themselves were being collected. Additionally, have Attr (contains a Value) and Bindings explicitly inherit from gc, even though Bindings is always allocated under `new (GC)`. Detected by running under GC_ENABLE_INCREMENTAL=1. Change-Id: Iacf13b34b5aa12e417ea87c9b46e2bf9199fdb26 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1544 Reviewed-by: glittershark <grfn@gws.fyi> Tested-by: BuildkiteCI
2020-08-01 r/1528 fix(3p/nix/libexpr): fix GC tracing in valueSizeKane York2-26/+29
Change-Id: I2f6bef7b090d44f50bd27fbd19b50f9cf100b238 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1506 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in>
2020-08-01 r/1527 feat(3p/nix): remove External values featureKane York5-113/+10
External values are only useful when using the plugin framework, which we are not interested in carrying forward. Reverts commit 320659b0cd161249c95e736c3fb309b1a73ea728 Change-Id: Ib4929c349bbb33f16224fc674e94c7b7d5953c6a Reviewed-on: https://cl.tvl.fyi/c/depot/+/1505 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in>
2020-08-01 r/1526 fix(3p/nix): Use a proper pointer in Env to carry with-attrsKane York2-5/+9
This eliminates the value-smuggling that would trip up the GC. Change-Id: I8057df78cf0bf6bea9faf1b44233aa9820ae44f5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1504 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in>
2020-08-01 r/1525 chore(3p/nix/libexpr): Cleanups and notes in eval.ccKane York2-3/+17
Add two more garbage-collection flags. Annotate how terrible tExternal is. Prepare to fix the smuggle casting in ExprWith. Add a static_cast. Change-Id: I20f980abc8cb192e094f539185900a6df5457c29 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1503 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in>
2020-08-01 r/1515 chore(3p/nix/hash): prefer StatusOr over throwing constructorKane York1-7/+10
The use of `unwrap_throw` can be used as a later grep target. Change-Id: I8c54ed90c4289f07aecb8a1393dd10204c8bce4e Reviewed-on: https://cl.tvl.fyi/c/depot/+/1493 Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: tazjin <mail@tazj.in> Tested-by: BuildkiteCI
2020-07-27 r/1495 fix(3p/nix): apply all clang-tidy fixesKane York12-108/+137
Change-Id: I265e763393422ee1881653527c91024458060825 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1432 Tested-by: BuildkiteCI Reviewed-by: tazjin <mail@tazj.in>
2020-07-24 r/1459 fix(3p/nix): fix all UninitializedObject warningsKane York1-1/+1
The cpptoml occurrences are ignored in CL 1419. Updates: #11 Change-Id: Ifb21c93b09c67e6b6a4c9e7089abffe7616569fd Reviewed-on: https://cl.tvl.fyi/c/depot/+/1407 Reviewed-by: glittershark <grfn@gws.fyi> Tested-by: BuildkiteCI
2020-07-23 r/1428 feat(3p/nix): remove allow-unsafe-native-code-during-evaluationAlyssa Ross1-5/+0
This formerly controlled access to builtins.exec and builtins.importNative, but both of those are gone now, so there's no need for this option any more. Change-Id: I6850cbd6be264fbfb1b209a60026dadbd0ba1232 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1341 Reviewed-by: glittershark <grfn@gws.fyi> Tested-by: BuildkiteCI
2020-07-23 r/1424 feat(3p/nix): remove builtins.importNativeAlyssa Ross2-55/+0
This is the shared object equivalent of builtins.exec, or a plugins equivalent accessible from the Nix language. Either way, since we don't have builtins.exec or plugins any more, I think it makes sense to remove this builtin. This will also allow us to drop the allow-unsafe-native-code-during-evaluation option, which formerly controlled whether builtins.exec and builtins.importNative were enabled. Cc: Griffin Smith <grfn@gws.fyi> Cc: Profpatsch <mail@profpatsch.de> Change-Id: I8993a8a79d559c102647308a2684c089bbc06713 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1340 Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: Profpatsch <mail@profpatsch.de> Tested-by: BuildkiteCI
2020-07-20 r/1408 chore(3p/nix): Remove support for pluginsGriffin Smith1-3/+1
Plugins seem to not really be used anywhere (I can find one plugin that's actually defined, and it doesn't seem very useful, especially since we got rid of builtins.exec) and their presence is adding additional complexity and potential sources of bugs to an already unsteady refactor. At some point we may want to bring back something *like* plugins, but their design will likely be different and it will definitely be after we have a functioning Nix again. Change-Id: I3bc40e55917f70bf260fbc208c1705e2e6a7c626 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1291 Tested-by: BuildkiteCI Reviewed-by: Alyssa Ross <hi@alyssa.is> Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-19 r/1403 fix(3p/nix): Fix all remaining compiler warningsVincent Ambo4-4/+15
This compiles under `-Wall -Werror`. The largest chunk of this change is `final` qualifiers for the various Nix CLI command structs, which inherit from a Command class that has more virtual functions than are implemented by each command. Change-Id: I0925e6e1a39013f026773db5816e4a77d50f3b4a Reviewed-on: https://cl.tvl.fyi/c/depot/+/1294 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi> Reviewed-by: kanepyork <rikingcoding@gmail.com>
2020-07-19 r/1402 chore(3p/nix): Update deprecated 'pure-parser' settingVincent Ambo1-1/+1
This setting was renamed to api.pure about 300 years ago and caused warnings to be issued. Change-Id: If883b0667c3afe67ae3d2a9950a796688cfbea7a Reviewed-on: https://cl.tvl.fyi/c/depot/+/1290 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-19 r/1400 refactor(3p/nix): Rename & undeprecate Bindings::lexicographicOrderVincent Ambo5-6/+10
The function is renamed to `SortedByKeys`, which is more descriptive, and annotated with a comment about what it is used for. The deprecation warning has been removed because this function is currently functionally required. Change-Id: I0ee3a76deff05f366feca9ddac8f38ab34bffbd0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1288 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-19 r/1393 fix(3p/nix): convert all holders of EvalState into gcKane York1-1/+1
Change-Id: Ia0d1fdd5c73941d02da0ca7152600f966cee5ccb Reviewed-on: https://cl.tvl.fyi/c/depot/+/1280 Tested-by: BuildkiteCI Reviewed-by: tazjin <mail@tazj.in>
2020-07-19 r/1392 refactor(3p/nix/libexpr): Move some code out of lexer.lVincent Ambo2-52/+53
Moves a function that is not dependent on the generated code over to parser.hh. This function also looks like it could be improved, but that is left as an exercise for the reader. Code that remains in lexer.l has been reformatted, while we're here. Change-Id: I9c26bb4eed0772a720d0715029e8bc10ab16ac38 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1279 Tested-by: BuildkiteCI Reviewed-by: Kane York <rikingcoding@gmail.com>
2020-07-19 r/1388 fix(3p/nix/libexpr): Make EvalState internals visible to GCVincent Ambo1-1/+1
Change-Id: Ia7de49acb859040429fdd8ab143d485589ac02d4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1277 Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: isomer <isomer@tvl.fyi> Reviewed-by: Kane York <rikingcoding@gmail.com> Tested-by: BuildkiteCI
2020-07-18 r/1377 refactor(3p/nix/libexpr): Back Nix lists with std::vectorVincent Ambo11-205/+141
This change does away with the previous special-casing of lists of certain element sizes, and the use of raw C-style arrays. Lists are now backed by a std::vector of nix::Value*, which uses the traceable GC allocator. This change is unfortunately quite noisy because the accessor methods were updated/removed accordingly, so all callsites of Nix-related lists have changed. For some operations in primops.cc where keeping the previous code structure would have been more difficult with a "proper" vector, the implementation has been replaced with std::vector methods. For example, list concatenation now uses appropriate range inserts. Anecdotally the performance of this is about equal, to even slightly better, than the previous implementation. All language tests pass and the depot paths I've used for testing still evaluate. Change-Id: Ib5eca6c0207429cb323a330c838c3a2200b2c693 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1266 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi> Reviewed-by: Kane York <rikingcoding@gmail.com> Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-18 r/1376 refactor(3p/nix/libexpr): Store nix::Env values in a std::vectorVincent Ambo2-9/+11
This has several advantages: * we can ensure that the vector is traced by the GC * we don't need to unsafely allocate memory to make an Env Note that there was previously a check about the size of the environment, but it's unclear why this was the case (git history yielded nothing interesting) and it seems to have no effect. Change-Id: I4998b879a728a6fb68e1bd187c521e2304e5047e Reviewed-on: https://cl.tvl.fyi/c/depot/+/1265 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi> Reviewed-by: Kane York <rikingcoding@gmail.com> Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-17 r/1354 feat(3p/nix): Add function to allocate a Value in traceable memoryEelco Dolstra3-0/+10
Backported from: https://github.com/NixOS/nix/commit/b3e5eea4a91400fb2a12aba4b07a94d03ba54605 https://github.com/NixOS/nix/commit/fcd048a526bd239fa615457e77d61d69d679bf03 Intentionally skipped because we have not backported the JSON changes: https://github.com/NixOS/nix/commit/9f46f54de4e55267df492456fc0393f74616366b Did not apply changes ni primops.cc, because those look suspect and are also based on something that we don't have in our tree. Change-Id: I837787ce9f2c90267bc39fce15177980d209d4e9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1253 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-17 r/1353 fix(3p/nix): ValueMap, VectorVector: Use traceable_allocatorEelco Dolstra1-2/+2
We want to *trace* the 'Value *' arrays, not garbage-collect them! Otherwise the vectors/maps can end up pointing to nowhere. Backported from: https://github.com/NixOS/nix/commit/10e17eaa5802a3c368eee8408821828072e76ba7 Change-Id: I30dc94caa80c9d982e7a14bc67ba2d065e8203aa Reviewed-on: https://cl.tvl.fyi/c/depot/+/1252 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-17 r/1346 chore(3p/nix): Remove builtins.execVincent Ambo2-45/+0
This is a bad idea, it shouldn't exist, nixpkgs doesn't use it. Change-Id: Ic4d1b936d8f059d5c40f0567af165b02427d7e36 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1241 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-16 r/1325 refactor(3p/nix/libexpr): Use Abseil collection types for parser stateVincent Ambo2-2/+13
Replaces the previous uses of the (ordered!) std::map and std::set with absl::flat_hash_{map|set}. After some careful reading it seems that there is actually no ordering dependency on these types, and the (drop-in) replacements perform slightly better. Overall this is not fixing a bottleneck, just a driveby thing. Change-Id: Ided695dc75676bd58515aa9382df0be0a09c565e Reviewed-on: https://cl.tvl.fyi/c/depot/+/1220 Reviewed-by: glittershark <grfn@gws.fyi> Reviewed-by: isomer <isomer@tvl.fyi> Tested-by: BuildkiteCI
2020-07-16 r/1323 refactor(3p/nix/libexpr): Move EvalState:* implementations out of parser.yVincent Ambo3-150/+156
Several definitions of functions declared in eval.hh were previously implemented in parser.y, this moves them over to parser.cc. While this still isn't a reasonable place to keep them, the long-term fix is more likely to be that eval.hh needs to be split up. Before we get to that point however, this already gives us the ability to use tooling with this code. Change-Id: If06fb655325fe281564047ffab0a0a640428a0ee Reviewed-on: https://cl.tvl.fyi/c/depot/+/1219 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-16 r/1322 refactor(3p/nix/libexpr): Extract static helpers out of parser.yVincent Ambo4-185/+202
Moves several of the static helper functions into a new parser.cc file. Once the rest of the code is usefully extracted, these will be moved to a private namespace. Change-Id: I0d7b53dcefe31bb5c6bad3ad7f5fcb48276bf799 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1218 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-16 r/1321 refactor(3p/nix/libexpr): Extract parser header out of Yacc fileVincent Ambo3-54/+49
First step (of many?) towards extracting all the inline code from the Yacc file and keeping it somewhere more accessible instead. Note that none of this code has previously been touched by a linter or formatter, pretty much ever, so as it is extracted it also undergoes similar changes to the whole codebase after the initial fork. Change-Id: If3b7181f22e3b3fd8c58dfa9befa7ee2896ea06d Reviewed-on: https://cl.tvl.fyi/c/depot/+/1217 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-16 r/1320 refactor(3p/nix/libexpr): Use range insertion to merge nix::BindingsVincent Ambo3-18/+25
Instead of manually iterating over the two bindings to be combined, this adds a new static method on the Bindings class which merges two attribute sets by calling the range insertion operator over them. In some anecdotal tests, this can lead to a ~10% speed bump - depending on the specific operation. Change-Id: I5dea03b0589a83a789d3a8a0fc81d0d9e6598371 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1216 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-16 r/1307 style(3p/nix): Minor style fix in DrvInfo::setMetaVincent Ambo1-3/+2
Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I0655ecc675239b3d90e5adc305c3f37c1a904cf5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1181 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-16 r/1306 refactor(3p/nix): Compare attribute sets by pointer equalityVincent Ambo1-0/+6
Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I418e9127c5d9d31559c59e461f17726ddbc051c4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1180 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-16 r/1305 refactor(3p/nix): Use a static empty Bindings for 0-element attrsVincent Ambo1-2/+11
A significant fraction of all created attribute sets are empty; hence this is an easy optimisation to make. Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I0884194d04c1ee95b2b239a253515f2152bc0856 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1179 Tested-by: BuildkiteCI Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-15 r/1298 refactor(3p/nix): Revert VectorBindings implementationVincent Ambo2-235/+30
This reverts parts of the CLs splitting the backing implementation for Bindings and moves back to only the BTreeMap-backed implementation. Our evaluation has indicated that the Vector-backed implementation does not match the performance of the plain array used upstream, and in my view the complexity introduced by it is not worth the relatively small (single-digit percentage) performance increase with a pivot-point close to the number of attributes yielded by stdenv.mkDerivation. Going forward we will trial implementations of attribute sets backed by HAMTs, and investigate other mechanisms of speeding up the language. Some changes from the previous CLs are retained, for example the removal of insert_or_assign and the passing of capacity. Change-Id: I6eb4b075b453949583360755055c21a29d7ff642 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1172 Reviewed-by: glittershark <grfn@gws.fyi> Tested-by: BuildkiteCI
2020-07-13 r/1289 refactor(3p/nix/libexpr): Backport upstream VectorBindings merge sortVincent Ambo1-17/+28
Since one of the two implementations essentially uses the same shape as the upstream Bindings, we backport their merge sort implementation to ensure that we're doing the same thing semantically. Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I0d865897991eec0c4dd84d9bd0415cd1ca437792 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1162 Tested-by: BuildkiteCI Reviewed-by: lukegb <lukegb@tvl.fyi> Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-13 r/1288 fix(3p/nix/libexpr): Remove default & copy constructors for VectorBindingsVincent Ambo1-1/+2
Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: Ief2e59d461452ce599abc63f6ebcfa07a7062491 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1161 Tested-by: BuildkiteCI Reviewed-by: lukegb <lukegb@tvl.fyi>
2020-07-13 r/1287 fix(3p/nix/libexpr): Fix post-increment operator in BindingsIteratorVincent Ambo1-1/+2
We accidentally returned the incremented iterator in the post-increment, this fixes it. Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I36c79eb56359bb12a78ad3489e7d7d2eb2053510 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1140 Tested-by: BuildkiteCI Reviewed-by: lukegb <lukegb@tvl.fyi> Reviewed-by: glittershark <grfn@gws.fyi>
2020-07-13 r/1286 refactor(3p/nix): Remove unused insert_or_assignGriffin Smith2-26/+0
This function in never called, so let's just remove it Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Change-Id: I79125866254d90dd0842bc86830d2103ac313cb6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1125 Tested-by: BuildkiteCI Reviewed-by: lukegb <lukegb@tvl.fyi> Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-13 r/1285 feat(3p/nix): Statically pass bindings capacity where possibleGriffin Smith5-31/+39
To aid in making the decision of where to (currently just statically) use a vector or btree as the backing implementation, add an extra constructor argument to Bindings::NewGC for a capacity, and use a (currently hardcoded at 32, for no good reason other than it felt like a reasonable number) pivot to switch between our possible backing implementations. Then, update all the call sites where it feels reasonable that we know the capacity statically to *pass* that capacity to the constructor. Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I1858c161301a1cd0e83aeeb9a58839378869e71d Reviewed-on: https://cl.tvl.fyi/c/depot/+/1124 Tested-by: BuildkiteCI Reviewed-by: lukegb <lukegb@tvl.fyi> Reviewed-by: isomer <isomer@tvl.fyi>
2020-07-13 r/1284 feat(3p/nix): Add vector-backed impl for BindingsGriffin Smith2-16/+150
Add an alternative impl of the now-abstract Bindings base class that is backed by a std::vector, somewhat similar but stylistically a little superior to the array-backed implementation in upstream nix. The underlying iterator type in BindingsIterator is now backed by a std::variant that we std::visit an overload over in order to implement the various bits of the iterator interface. Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: I7fbd1f4d5c449e2f9b82102a701b0bacd5e80672 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1123 Tested-by: BuildkiteCI Reviewed-by: tazjin <mail@tazj.in>
2020-07-13 r/1283 refactor(3p/nix): Abstract away concrete bindings implGriffin Smith2-24/+94
To pave the way for the thing we want to do eventually which is use a linear-time array for bindings (aka attribute sets) that are statically known to be small enough to get a performance benefit from doing so, make the Bindings class abstract, and define a BTreeBindings class that inherits from it and is (currently always) returned from the static initializer. The idea is that we'll have an ArrayBindings class as well later that we can dispatch to conditionally based on an optional "capacity" parameter or something like that. There was some difficulty here in getting the iterator to work - the approach we settled on ended up making a concrete BindingsIterator class which will wrap a std::variant of either a btree iterator or something else later, but right now just wraps a btree iterator. Paired-With: Luke Granger-Brown <git@lukegb.com> Paired-With: Vincent Ambo <mail@tazj.in> Paired-With: Perry Lorier <isomer@tvl.fyi> Change-Id: Ie02ca5a1c55e8ebf99ab1e957110bd9284278907 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1121 Tested-by: BuildkiteCI Reviewed-by: isomer <isomer@tvl.fyi>