| commit | 5abe7aa53bf9abe78c9446c1a4c0b515eb37ec6e | |
| author | Jake Bailey (Hacklang) <jakebailey@fb.com> | |
| Fri, 18 Oct 2019 14:34:23 +0000 (18 07:34 -0700) | ||
| committer | Facebook Github Bot <facebook-github-bot@users.noreply.github.com> | |
| Fri, 18 Oct 2019 14:36:17 +0000 (18 07:36 -0700) | ||
| tree | d5b9e478a8c5d6bb49f8d9fbbb402317139d4d01 | treesnapshot (tar.gz zip) |
| parent | c8b8f32bea2f90226a3c372eb0913f3f054399a8 | commitdiff |
Encapsulate intrusive OCaml value caching in RcOc
Summary:
The PositionedToken type wraps its implementation in an `Rc`, since tokens are shared between many nodes in a positioned CST (since each node holds on to references to the leading token in its first child and the trailing token in its last child). Originally, we preserved this sharing while converting to OCaml by maintaining a HashMap from the token address to the converted OCaml value in a SerializationContext. In {D16541139}, we improved performance by storing this mapping intrusively instead. AIUI, we used `mem::transmute` here rather than `Cell` because of the use of `catch_unwind` in the parser--since `Cell` does not implement `RefUnwindSafe`, we used unsafe code to hide our use of internal mutability from the compiler.
In {D17772800}, we are considering adding fields to RelativePath to implement the same intrusive-caching strategy. I'm proposing here that instead, we encapsulate this pattern in a type named `RcOc`, and use `RcOc` rather than `Rc` in situations where conversion-to-OCaml performance is critical. I did not find that `Cell` not being `RefUnwindSafe` was a problem for this implementation, so this diff just uses `Cell`s rather than transmutes.
Reviewed By: shiqicao, losvald
Differential Revision: D17955577
fbshipit-source-id: b33567f54914ac569ba26b61dcb6ca0d32e1d933
Summary:
The PositionedToken type wraps its implementation in an `Rc`, since tokens are shared between many nodes in a positioned CST (since each node holds on to references to the leading token in its first child and the trailing token in its last child). Originally, we preserved this sharing while converting to OCaml by maintaining a HashMap from the token address to the converted OCaml value in a SerializationContext. In {D16541139}, we improved performance by storing this mapping intrusively instead. AIUI, we used `mem::transmute` here rather than `Cell` because of the use of `catch_unwind` in the parser--since `Cell` does not implement `RefUnwindSafe`, we used unsafe code to hide our use of internal mutability from the compiler.
In {D17772800}, we are considering adding fields to RelativePath to implement the same intrusive-caching strategy. I'm proposing here that instead, we encapsulate this pattern in a type named `RcOc`, and use `RcOc` rather than `Rc` in situations where conversion-to-OCaml performance is critical. I did not find that `Cell` not being `RefUnwindSafe` was a problem for this implementation, so this diff just uses `Cell`s rather than transmutes.
Reviewed By: shiqicao, losvald
Differential Revision: D17955577
fbshipit-source-id: b33567f54914ac569ba26b61dcb6ca0d32e1d933