flow-control: Add cache node optimization.#8115
Conversation
b7ee27c to
0a909b4
Compare
80b3efb to
95b9ca7
Compare
orizi
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @liorgold2)
crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 12 at r1 (raw file):
/// identically. pub struct Cache<Input> { cache: RefCell<UnorderedHashMap<Input, NodeId>>,
doc why refcell.
95b9ca7 to
b1ca544
Compare
b1ca544 to
f61aad1
Compare
liorgold2
left a comment
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi)
crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 12 at r1 (raw file):
Previously, orizi wrote…
doc why refcell.
Done.
orizi
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @liorgold2)
crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 15 at r2 (raw file):
// // The cache is wrapped in a `RefCell` to allow modifying it without holding a `&mut` to it // (which would complicate its usage).
Suggestion:
/// A map from input to the cached result.
///
/// The cache is wrapped in a `RefCell` to allow modifying it without holding a `&mut` to it
/// (which would complicate its usage).crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 15 at r2 (raw file):
// // The cache is wrapped in a `RefCell` to allow modifying it without holding a `&mut` to it // (which would complicate its usage).
this is pretty much the definition of a refcell - why is this specifically more difficult?
i now understand - it is since it is a cache - and not an explicitly built mapper - so you consider caching as a non-mutating action. (i think i'd rather see something similar to this line)
but couldn't you just make it a mut fn?
you have it as a local variable in both usecases.
Code quote:
// The cache is wrapped in a `RefCell` to allow modifying it without holding a `&mut` to it
// (which would complicate its usage).commit-id:edbf06d2
f61aad1 to
d8c5afb
Compare
liorgold2
left a comment
There was a problem hiding this comment.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @orizi)
crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 15 at r2 (raw file):
Previously, orizi wrote…
this is pretty much the definition of a refcell - why is this specifically more difficult?
i now understand - it is since it is a cache - and not an explicitly built mapper - so you consider caching as a non-mutating action. (i think i'd rather see something similar to this line)but couldn't you just make it a mut fn?
you have it as a local variable in both usecases.
Nice, works :)
crates/cairo-lang-lowering/src/lower/flow_control/create_graph/cache.rs line 15 at r2 (raw file):
// // The cache is wrapped in a `RefCell` to allow modifying it without holding a `&mut` to it // (which would complicate its usage).
Done.
orizi
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @liorgold2)
Stack: