depot/tvix/eval/src/upvalues.rs, branch refs/r/8348 monorepo for the virus lounge http://code.tvl.fyi/depot/atom?h=refs%2Fr%2F8348 2022-11-04T00:30:13+00:00 fix(tvix/eval): remove impl PartialEq for Value 2022-11-04T00:30:13+00:00 Adam Joseph adam@westernsemico.com 2022-11-01T00:13:38+00:00 urn:sha1:06494742062e77036827dfc7c91dea507b44447f It isn't possible to implement PartialEq properly for Value, because any sensible implementation needs to force() thunks, which cannot be done without a `&mut VM`. The existing derive(PartialEq) has false negatives, which caused the bug which cl/7142 fixed. Fortunately that bug was easy to find, but a silent false negative deep within the bowels of nixpkgs could be a real nightmare to hunt down. Let's just remove the PartialEq impl for Value, and the other derive(PartialEq)'s that depend on it. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: Iacd3726fefc7fc1edadcd7e9b586e04cf8466775 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7144 Reviewed-by: kanepyork <rikingcoding@gmail.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI feat(tvix/eval): deduplicate overlap between Closure and Thunk 2022-10-19T10:38:54+00:00 Adam Joseph adam@westernsemico.com 2022-10-15T23:10:10+00:00 urn:sha1:d978b556e6d3189760e36fbc0fefe683618cf6ad This commit deduplicates the Thunk-like functionality from Closure and unifies it with Thunk. Specifically, we now have one and only one way of breaking reference cycles in the Value-graph: Thunk. No other variant contains a RefCell. This should make it easier to reason about the behavior of the VM. InnerClosure and UpvaluesCarrier are no longer necessary. This refactoring allowed an improvement in code generation: `Rc<RefCell<>>`s are now created only for closures which do not have self-references or deferred upvalues, instead of for all closures. OpClosure has been split into two separate opcodes: - OpClosure creates non-recursive closures with no deferred upvalues. The VM will not create an `Rc<RefCell<>>` when executing this instruction. - OpThunkClosure is used for closures with self-references or deferred upvalues. The VM will create a Thunk when executing this opcode, but the Thunk will start out already in the `ThunkRepr::Evaluated` state, rather than in the `ThunkRepr::Suspeneded` state. To avoid confusion, OpThunk has been renamed OpThunkSuspended. Thanks to @sterni for suggesting that all this could be done without adding an additional variant to ThunkRepr. This does however mean that there will be mutating accesses to `ThunkRepr::Evaluated`, which was not previously the case. The field `is_finalised:bool` has been added to `Closure` to ensure that these mutating accesses are performed only on finalised Closures. Both the check and the field are present only if `#[cfg(debug_assertions)]`. Change-Id: I04131501029772f30e28da8281d864427685097f Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> docs(tvix/eval): upvalues.rs: define "upvalue", comment with_stack 2022-10-18T09:38:16+00:00 Adam Joseph adam@westernsemico.com 2022-10-13T20:41:30+00:00 urn:sha1:4b01e594d5d5cb806f6fe6eef1c30069748369cd This commit adds a comment for the benefit of non-Lua-users explaining what an upvalue is. It also adds a comment explaining what `with_stack` does, including a brief reminder of nix's slightly-unusual-but-well-justified handling of this construct. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: If6d0e133292451af906e1c728e34010f99be8c7c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7007 Reviewed-by: j4m3s <james.landrein@gmail.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI refactor(tvix/eval): Don't (ab)use PartialEq for Nix equality 2022-09-18T22:03:41+00:00 Griffin Smith root@gws.fyi 2022-09-18T18:04:40+00:00 urn:sha1:915ff5ac2a180cbd736ce8404c46566a14d484ba Using rust's PartialEq trait to implement Nix equality semantics is reasonably fraught with peril, both because the actual laws are different than what nix expects, and (more importantly) because certain things actually require extra context to compare for equality (for example, thunks need to be forced). This converts the manual PartialEq impl for Value (and all its descendants) to a *derived* PartialEq impl (which requires a lot of extra PartialEq derives on miscellanious other types within the codebase), and converts the previous nix-semantics equality comparison into a new `nix_eq` method. This returns an EvalResult, even though it can't currently return an error, to allow it to fail when eg forcing thunks (which it will do soon). Since the PartialEq impls for Value and NixAttrs are now quite boring, this converts the generated proptests for those into handwritten ones that cover `nix_eq` instead Change-Id: If3da7171f88c22eda5b7a60030d8b00c3b76f672 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6650 Autosubmit: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> refactor(tvix/eval): capture entire with_stack in upvalues 2022-09-11T12:26:23+00:00 Vincent Ambo mail@tazj.in 2022-09-06T20:13:48+00:00 urn:sha1:07ea30370e887b16228af0dccbe126010cce9e25 This completely rewrites the handling of "dynamic upvalues" to, instead of resolving them at thunk/closure instantiation time (which forces some values too early), capture the entire with stack of parent contexts if it exists. There are a couple of things in here that could be written more efficiently, but I'm first working through this to get to a bug related to with + recursion and the code complexity of some of the optimisations is distracting. Change-Id: Ia538e06c9146e3bf8decb9adf02dd726d2c651cf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6486 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> refactor(tvix/eval): introduce Upvalues struct in closures & thunks 2022-09-11T12:16:46+00:00 Vincent Ambo mail@tazj.in 2022-09-06T19:08:25+00:00 urn:sha1:d75b207a63492cb120bcdd918fcc4178dca2bc36 This struct will be responsible for tracking upvalues (and is a convenient place to introduce optimisations for reducing value clones) instead of a plain value vector. The main motivation for this is that the upvalues will have to capture the `with`-stack fully and I want to avoid duplicating the logic for this between the two capturing types. Change-Id: I6654f8739fc2e04ca046e6667d4a015f51724e99 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6485 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> refactor(tvix/eval): introduce UpvalueCarrier trait 2022-09-06T14:58:52+00:00 Vincent Ambo mail@tazj.in 2022-08-29T15:07:58+00:00 urn:sha1:25c62dd0ef70a37915957bb1eb8ba3f4885c7aad This trait abstracts over the commonalities of upvalue handling between closures and thunks. It allows the VM to simplify the code used for setting up upvalues, without duplicating between the two different types. Note that this does not yet refactor the VM code to optimally make use of this. Change-Id: If8de5181f26ae1fa00d554f1ae6ea473ee4b6070 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6347 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
This XML file does not appear to have any style information associated with it. The document tree is shown below.
<feed xmlns="http://www.w3.org/2005/Atom">
<title>depot/tvix/eval/src/upvalues.rs, branch refs/r/8348</title>
<subtitle>monorepo for the virus lounge</subtitle>
<id>http://code.tvl.fyi/depot/atom?h=refs%2Fr%2F8348</id>
<link rel="self" href="http://code.tvl.fyi/depot/atom?h=refs%2Fr%2F8348"/>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/"/>
<updated>2022-11-04T00:30:13+00:00</updated>
<entry>
<title>fix(tvix/eval): remove impl PartialEq for Value</title>
<updated>2022-11-04T00:30:13+00:00</updated>
<author>
<name>Adam Joseph</name>
<email>adam@westernsemico.com</email>
</author>
<published>2022-11-01T00:13:38+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=06494742062e77036827dfc7c91dea507b44447f"/>
<id>urn:sha1:06494742062e77036827dfc7c91dea507b44447f</id>
<content type="text"> It isn't possible to implement PartialEq properly for Value, because any sensible implementation needs to force() thunks, which cannot be done without a `&mut VM`. The existing derive(PartialEq) has false negatives, which caused the bug which cl/7142 fixed. Fortunately that bug was easy to find, but a silent false negative deep within the bowels of nixpkgs could be a real nightmare to hunt down. Let's just remove the PartialEq impl for Value, and the other derive(PartialEq)'s that depend on it. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: Iacd3726fefc7fc1edadcd7e9b586e04cf8466775 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7144 Reviewed-by: kanepyork <rikingcoding@gmail.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI </content>
</entry>
<entry>
<title>feat(tvix/eval): deduplicate overlap between Closure and Thunk</title>
<updated>2022-10-19T10:38:54+00:00</updated>
<author>
<name>Adam Joseph</name>
<email>adam@westernsemico.com</email>
</author>
<published>2022-10-15T23:10:10+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=d978b556e6d3189760e36fbc0fefe683618cf6ad"/>
<id>urn:sha1:d978b556e6d3189760e36fbc0fefe683618cf6ad</id>
<content type="text"> This commit deduplicates the Thunk-like functionality from Closure and unifies it with Thunk. Specifically, we now have one and only one way of breaking reference cycles in the Value-graph: Thunk. No other variant contains a RefCell. This should make it easier to reason about the behavior of the VM. InnerClosure and UpvaluesCarrier are no longer necessary. This refactoring allowed an improvement in code generation: `Rc<RefCell<>>`s are now created only for closures which do not have self-references or deferred upvalues, instead of for all closures. OpClosure has been split into two separate opcodes: - OpClosure creates non-recursive closures with no deferred upvalues. The VM will not create an `Rc<RefCell<>>` when executing this instruction. - OpThunkClosure is used for closures with self-references or deferred upvalues. The VM will create a Thunk when executing this opcode, but the Thunk will start out already in the `ThunkRepr::Evaluated` state, rather than in the `ThunkRepr::Suspeneded` state. To avoid confusion, OpThunk has been renamed OpThunkSuspended. Thanks to @sterni for suggesting that all this could be done without adding an additional variant to ThunkRepr. This does however mean that there will be mutating accesses to `ThunkRepr::Evaluated`, which was not previously the case. The field `is_finalised:bool` has been added to `Closure` to ensure that these mutating accesses are performed only on finalised Closures. Both the check and the field are present only if `#[cfg(debug_assertions)]`. Change-Id: I04131501029772f30e28da8281d864427685097f Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> </content>
</entry>
<entry>
<title>docs(tvix/eval): upvalues.rs: define "upvalue", comment with_stack</title>
<updated>2022-10-18T09:38:16+00:00</updated>
<author>
<name>Adam Joseph</name>
<email>adam@westernsemico.com</email>
</author>
<published>2022-10-13T20:41:30+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=4b01e594d5d5cb806f6fe6eef1c30069748369cd"/>
<id>urn:sha1:4b01e594d5d5cb806f6fe6eef1c30069748369cd</id>
<content type="text"> This commit adds a comment for the benefit of non-Lua-users explaining what an upvalue is. It also adds a comment explaining what `with_stack` does, including a brief reminder of nix's slightly-unusual-but-well-justified handling of this construct. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: If6d0e133292451af906e1c728e34010f99be8c7c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7007 Reviewed-by: j4m3s <james.landrein@gmail.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI </content>
</entry>
<entry>
<title>refactor(tvix/eval): Don't (ab)use PartialEq for Nix equality</title>
<updated>2022-09-18T22:03:41+00:00</updated>
<author>
<name>Griffin Smith</name>
<email>root@gws.fyi</email>
</author>
<published>2022-09-18T18:04:40+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=915ff5ac2a180cbd736ce8404c46566a14d484ba"/>
<id>urn:sha1:915ff5ac2a180cbd736ce8404c46566a14d484ba</id>
<content type="text"> Using rust's PartialEq trait to implement Nix equality semantics is reasonably fraught with peril, both because the actual laws are different than what nix expects, and (more importantly) because certain things actually require extra context to compare for equality (for example, thunks need to be forced). This converts the manual PartialEq impl for Value (and all its descendants) to a *derived* PartialEq impl (which requires a lot of extra PartialEq derives on miscellanious other types within the codebase), and converts the previous nix-semantics equality comparison into a new `nix_eq` method. This returns an EvalResult, even though it can't currently return an error, to allow it to fail when eg forcing thunks (which it will do soon). Since the PartialEq impls for Value and NixAttrs are now quite boring, this converts the generated proptests for those into handwritten ones that cover `nix_eq` instead Change-Id: If3da7171f88c22eda5b7a60030d8b00c3b76f672 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6650 Autosubmit: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> </content>
</entry>
<entry>
<title>refactor(tvix/eval): capture entire with_stack in upvalues</title>
<updated>2022-09-11T12:26:23+00:00</updated>
<author>
<name>Vincent Ambo</name>
<email>mail@tazj.in</email>
</author>
<published>2022-09-06T20:13:48+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=07ea30370e887b16228af0dccbe126010cce9e25"/>
<id>urn:sha1:07ea30370e887b16228af0dccbe126010cce9e25</id>
<content type="text"> This completely rewrites the handling of "dynamic upvalues" to, instead of resolving them at thunk/closure instantiation time (which forces some values too early), capture the entire with stack of parent contexts if it exists. There are a couple of things in here that could be written more efficiently, but I'm first working through this to get to a bug related to with + recursion and the code complexity of some of the optimisations is distracting. Change-Id: Ia538e06c9146e3bf8decb9adf02dd726d2c651cf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6486 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> </content>
</entry>
<entry>
<title>refactor(tvix/eval): introduce Upvalues struct in closures & thunks</title>
<updated>2022-09-11T12:16:46+00:00</updated>
<author>
<name>Vincent Ambo</name>
<email>mail@tazj.in</email>
</author>
<published>2022-09-06T19:08:25+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=d75b207a63492cb120bcdd918fcc4178dca2bc36"/>
<id>urn:sha1:d75b207a63492cb120bcdd918fcc4178dca2bc36</id>
<content type="text"> This struct will be responsible for tracking upvalues (and is a convenient place to introduce optimisations for reducing value clones) instead of a plain value vector. The main motivation for this is that the upvalues will have to capture the `with`-stack fully and I want to avoid duplicating the logic for this between the two capturing types. Change-Id: I6654f8739fc2e04ca046e6667d4a015f51724e99 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6485 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> </content>
</entry>
<entry>
<title>refactor(tvix/eval): introduce UpvalueCarrier trait</title>
<updated>2022-09-06T14:58:52+00:00</updated>
<author>
<name>Vincent Ambo</name>
<email>mail@tazj.in</email>
</author>
<published>2022-08-29T15:07:58+00:00</published>
<link rel="alternate" type="text/html" href="http://code.tvl.fyi/commit/?id=25c62dd0ef70a37915957bb1eb8ba3f4885c7aad"/>
<id>urn:sha1:25c62dd0ef70a37915957bb1eb8ba3f4885c7aad</id>
<content type="text"> This trait abstracts over the commonalities of upvalue handling between closures and thunks. It allows the VM to simplify the code used for setting up upvalues, without duplicating between the two different types. Note that this does not yet refactor the VM code to optimally make use of this. Change-Id: If8de5181f26ae1fa00d554f1ae6ea473ee4b6070 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6347 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> </content>
</entry>
</feed>