| commit | 284d0110b0e642b6f1ea9980b8c54ed4bafd3965 | |
| author | Jake Bailey (Hacklang) <jakebailey@fb.com> | |
| Wed, 5 Aug 2020 18:42:03 +0000 (5 11:42 -0700) | ||
| committer | Facebook GitHub Bot <facebook-github-bot@users.noreply.github.com> | |
| Wed, 5 Aug 2020 19:06:02 +0000 (5 12:06 -0700) | ||
| tree | 9e4b6ef1c85818e273dbbe2bf1984448fb0c73df | treesnapshot (tar.gz zip) |
| parent | 109831deea0b6cd5575fd1f4d3f625cc287c64a5 | commitdiff |
Use List instead of Rc<AssocListMut> in direct decl parser
Summary:
We currently aggregate the decls from a file being parsed in a mutable association list. Since our SmartConstructors state may be cloned (if the parser performs some speculative parsing, from which it may want to backtrack), we put the mutable list behind an Rc. When we want to add a parsed declaration, we use Rc::make_mut. If the refcount of the Rc is 1, make_mut will give us a mutable reference to the inner list. If it's greater than 1, make_mut will clone the inner list before giving us a mutable reference. This implementation was chosen on the assumption that we are generally not performing a speculative parse when we complete parsing of a toplevel declaration.
It turns out that this assumption is not true as often as we'd thought. There are several circumstances where the parser holds on to a previous checkpoint while it completes parsing of a toplevel declaration, so our refcount here is often greater than 1, leading to unnecessary clones. These waste time and needlessly inflate the size of our arena. We could deal with this by carefully auditing the FFP entrypoints and declaration parser, ensuring that only one copy of the parser state exists when we finish parsing each toplevel declaration. But this invariant would be fragile--it would be easy for someone to accidentally regress direct decl parser performance by holding on to a parser state copy for too long. It's more robust to use an immutable data structure to aggregate parsed decls. This diff uses an arena-allocated linked list type to do so.
Reviewed By: arxanas
Differential Revision: D22796491
fbshipit-source-id: c0ab98ba61b02ec5a1a0e54fcdaa11bd1aa8bc02
Summary:
We currently aggregate the decls from a file being parsed in a mutable association list. Since our SmartConstructors state may be cloned (if the parser performs some speculative parsing, from which it may want to backtrack), we put the mutable list behind an Rc. When we want to add a parsed declaration, we use Rc::make_mut. If the refcount of the Rc is 1, make_mut will give us a mutable reference to the inner list. If it's greater than 1, make_mut will clone the inner list before giving us a mutable reference. This implementation was chosen on the assumption that we are generally not performing a speculative parse when we complete parsing of a toplevel declaration.
It turns out that this assumption is not true as often as we'd thought. There are several circumstances where the parser holds on to a previous checkpoint while it completes parsing of a toplevel declaration, so our refcount here is often greater than 1, leading to unnecessary clones. These waste time and needlessly inflate the size of our arena. We could deal with this by carefully auditing the FFP entrypoints and declaration parser, ensuring that only one copy of the parser state exists when we finish parsing each toplevel declaration. But this invariant would be fragile--it would be easy for someone to accidentally regress direct decl parser performance by holding on to a parser state copy for too long. It's more robust to use an immutable data structure to aggregate parsed decls. This diff uses an arena-allocated linked list type to do so.
Reviewed By: arxanas
Differential Revision: D22796491
fbshipit-source-id: c0ab98ba61b02ec5a1a0e54fcdaa11bd1aa8bc02