add helper methods to project MemoRef on some types #684

ch1ffa · 2025-09-23T12:56:58Z

Sometimes we need to use Ok value from MemoRef<Result<T,E>> without cloning because it is supposed to be passed as an argument to another memoized function as MemoRef<T> for performance reasons. This PR allows us to transform MemoRef<Result<T,E>> to Result<MemoRef<T>,E> without cloning the whole Result struct - only Error is cloned which is usually used with ? operator.

Same applies to MemoRef<Option<T>> and MemoRef<(T1, T2)>. And we also can chain this calls (up to 4) like try_ok()?.split().

I have to remove optimisation when MemoRef skips interning as param, but it's not a big deal - it's cheap to hash and copy.

NOTE: stacked on #683

rbalicki2

Will have to re-read and review more deeply. This is awesome!!! Was not expecting this level of shenanigans 🤩

rbalicki2

Partial review, overall, LGTM and I just want to reason about the unit tests and other details

rbalicki2 · 2025-10-02T02:41:02Z

crates/pico/src/memo_ref.rs

+
+#[inline(always)]
+fn step_identity(value: &dyn Any) -> &dyn Any {
+    value


this can panic, right? The len should always stop before we reach a step_identity, right?

array is always initialised with step_identity function pointers. But because we use len: 0 it's never called. If somehow there is a weird bug that increases len, then step_identity is almost noop. The only optimisation I believe we can apply here is using MaybeUninit, but it is tiny.

rbalicki2 · 2025-10-02T02:42:08Z

crates/pico/src/memo_ref.rs

+            db: self.db,
+            derived_node_id: self.derived_node_id,
+            projectors: self.projectors,
+            projectors_len: self.projectors_len,


it would be cleaner, I think, to just say self.projectors_len + 1. Wdyt?

rbalicki2 · 2025-10-02T02:43:37Z

crates/pico/src/memo_ref.rs

    pub(crate) db: *const dyn DatabaseDyn,
    pub(crate) derived_node_id: DerivedNodeId,
+    projectors: [MemoRefProjectorStep; MAX_PROJECTOR_STEPS],
+    projectors_len: u8,


can we add some comments here explaining what's happening?

rbalicki2 · 2025-10-02T02:47:01Z

crates/pico/src/memo_ref.rs

 pub struct MemoRef<T> {
    pub(crate) db: *const dyn DatabaseDyn,
    pub(crate) derived_node_id: DerivedNodeId,
+    projectors: [MemoRefProjectorStep; MAX_PROJECTOR_STEPS],


I think that the common case is that projectors_len is 0. So, perhaps it would be more performant to have an projectors: Option<...> (and no projectors_len)? (Do you think this is true? I'm not 100% convinced)

If it's indeed true that the common case is 0/None, then perhaps we don't need a fixed-length array, but can instead have a Vec<...>.

Oh also, Vec of length zero doesn't heap allocate anything, so perhaps we just have projectors: Vec<...>?

we need len to properly iterate over projectors chain and avoid calling unnecessary identity_step. I don't think Option gives us anything here, there is still a check for None/Some and it is the same as check for len in for loop.

rbalicki2 · 2025-10-02T02:51:17Z

crates/pico/src/memo_ref.rs

+        for step in &self.projectors[..len] {
+            any_ref = (step)(any_ref);
+        }
+        any_ref


hm. Can we optimize the case where len == 0 and avoid the any_ref = any_ref as dyn Any; any_ref.downcast_ref()?

I think we can check for len == 0 and avoid additional pointer copy and bound check.

rbalicki2

more nits/comments.

rbalicki2 · 2025-10-03T02:22:20Z

crates/pico/tests/try_ok.rs

+}
+
+#[derive(Debug, Clone, Error, PartialEq, Eq)]
+enum ProcessInputError {


not that I'm opposed to it per se, but does deriving Error get us anything in this test?

Yeah, we can just use a struct here. Because it is a Result I just without any thought used Error lol

rbalicki2 · 2025-10-03T02:34:48Z

crates/pico/tests/try_ok.rs

+    });
+
+    let result = process_input(&db, id).unwrap();
+    assert_eq!(*consume_result(&db, result), 'a');


What are we testing in this test that we aren't testing in try_ok_projects_ok_value?

we check if memo_ref that were passed as an argument is correctly cloned from the store with proper projector

rbalicki2 · 2025-10-03T02:36:30Z

crates/pico/tests/try_ok_split.rs

+}
+
+#[memo]
+fn ok_pair_value(_db: &TestDatabase) -> Result<(PanicOnClone, PanicOnClone), ()> {


QQ: what need do we have for tuples? Is that a specific, concrete thing we need? (Not opposed, just wondering)

we use it after parse_graphql_schema, because we need to clone one part and pass the second one as memo_ref

rbalicki2 · 2025-10-03T02:37:37Z

crates/pico/tests/try_some.rs

+    let db = TestDatabase::default();
+
+    let some_ref = some_value(&db);
+    let Some(inner) = some_ref.try_some() else {


nit: .expect(...)?

rbalicki2 · 2025-10-03T02:41:26Z

crates/pico/src/memo_ref.rs

            db: db as *const _ as *const dyn DatabaseDyn,
            derived_node_id,
+            projectors: [step_identity; MAX_PROJECTOR_STEPS],
+            projectors_len: 0,


Coming back to this... if the typical thing is len == 0 (is it?) then, perhaps we can take inspiration from how iterators work: https://doc.rust-lang.org/src/core/iter/adapters/map.rs.html#63 they are structs that wrap the parent, and have impl MemoRef<Item = T> everywhere? Then, we need only Map and FlatMap structs that wrap the underlying memo ref.

Is that doable? Worse than this solution? I don't know. I'm just pattern-matching on how stuff is done elsewhere in Rust

ch1ffa force-pushed the add-try-ok-to-result-memoref branch from 9840abf to 18564b2 Compare September 23, 2025 14:59

ch1ffa changed the title ~~Add try_ok method to MemoRef<Result<T, E>>~~ Add helper methods to project MemoRef on some types Sep 23, 2025

ch1ffa changed the title ~~Add helper methods to project MemoRef on some types~~ WIP: add helper methods to project MemoRef on some types Sep 23, 2025

ch1ffa force-pushed the add-try-ok-to-result-memoref branch from 18564b2 to 6e230c8 Compare September 23, 2025 16:09

ch1ffa changed the title ~~WIP: add helper methods to project MemoRef on some types~~ add helper methods to project MemoRef on some types Sep 23, 2025

rbalicki2 reviewed Sep 23, 2025

View reviewed changes

ch1ffa force-pushed the add-try-ok-to-result-memoref branch from 6e230c8 to d10852f Compare September 24, 2025 09:50

rbalicki2 force-pushed the add-try-ok-to-result-memoref branch from d10852f to f86b863 Compare October 2, 2025 02:28

rbalicki2 reviewed Oct 2, 2025

View reviewed changes

rbalicki2 reviewed Oct 3, 2025

View reviewed changes

ch1ffa added 2 commits October 3, 2025 17:12

add methods to project MemoRef

b567639

add len check

dc7f6c7

ch1ffa force-pushed the add-try-ok-to-result-memoref branch from f86b863 to dc7f6c7 Compare October 3, 2025 15:33

add helper methods to project MemoRef on some types #684

Are you sure you want to change the base?

add helper methods to project MemoRef on some types #684

Uh oh!

Conversation

ch1ffa commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbalicki2 left a comment

Choose a reason for hiding this comment

Uh oh!

rbalicki2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbalicki2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbalicki2 Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ch1ffa commented Sep 23, 2025 •

edited

Loading

rbalicki2 Oct 3, 2025 •

edited

Loading