Unexpected memo wrap (optimization?) causes a computation to be created outside of createRoot

[deluksic]

Describe the bug

https://playground.solidjs.com/anonymous/353e9151-5770-4d67-a3a1-5fa598920e7b

Unfortunately, the playground is not really showing the problem because it uses production solid, I think. But in the output tab you can see the _$memo being applied, presumably as an optimization. However, this causes a problem when such prop is used outside of roots, in events etc.

It is not reproducible in playground, but this is what I see locally:

Your Example Website or App

https://playground.solidjs.com/anonymous/353e9151-5770-4d67-a3a1-5fa598920e7b (Edit to fix a typo)

Steps to Reproduce the Bug or Issue

1. Go to output tab
2. Notice the added _$memo

Expected behavior

I would expect I can just use props. in events as a normal read.

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• Browser: [e.g. Chrome, Safari, Firefox]
• Version: [e.g. 91.1]

Additional context

Easy workaround is to extract the expression into a lambda above the return statement.

[deluksic]

Now that I'm looking at this again, I'm not sure how this helps at all. Wouldn't createMemo be called on every prop access, which would negate memoization?

[ryansolid]

@deluksic. No, because while ever access does create it, more often than not in Solid you access something once.. So the value here is that it doesn't cause the parent to re-evaluate while still truthy.. if count is still smaller than 5 so to speak the memo guards re-running. Yes in cases where there is a condition in a condition which causes it to be recreated then we are doing some extra book-keeping, or in this case where in an event handler we don't actually want a Memo to be created, but this gives us confidence that our condition won't cause the observing scope to re-run unnecessarily.

[deluksic]

Hm, I don't think I understand yet. Say we have this code

<MyComponent
  count={
    (console.log("checked"), count() > 0)
      ? Infinity
      : (console.log("accessed"), count())
  }
>

Which log is supposed to run less often? I see both checked and accessed on each prop.count access even when count === 0 and does not change.

[mizulu]

@deluksic

this how it is compiled:

  return _$createComponent(MyComponent, {
    get count() {
      return _$memo(() => !!(console.log("checked"), count() > 0))() ? Infinity : (console.log("accessed"), count());
    }
  });

every time you access the count prop in the component the get count() will be called
a new memo is created, and evaluated , and so you are going to see both logged

after the memo is created the "checked" should show on every count() signal change
but the "accessed" will only show on initial falsy condition, or when the condition of the memo goes from true to false.

[ryansolid]

Oh I get what you mean.. because the same accessor is accessed both in the condition and the result then the memoization is pointless. So yes in that case the optimization could be skipped. It would require a bit more analysis but it could be doable. But in general in many cases you don't both reference count in the condition and result. Like https://playground.solidjs.com/anonymous/b5568b57-42d5-4f76-b1b5-9f877a362831. In which case this is doing good work.

[deluksic]

I think I'm starting to understand what my confusion was. Would it be fair to say that this optimization is aimed at when there's JSX in the branches? I can't think of an example where usual signal logic / effects benefits from this memoization, because they would need to read props and as soon as they do, a new _$memo is created.

For example, it won't prevent reruns of normal effects:
https://playground.solidjs.com/anonymous/55a41fe8-5b55-477e-8961-34d901359385

[mizulu]

Yes the optimization is aimed at JSX / rendering
See "&& intrusive boolean casting memo" #2471 discussion
this discussion was more about && but applicable for ternary operator / conditional rendering

additionally any getter that may produce JSX on access will need handling similar
to props.childern, to ensure no over creation. I wouldn't put props.children in a createEffect

The optimization only apply when there is a function call

count()>1 && Date.now()            // Memo
count()> 1 ? Date.now()  : "B"     // Memo 
count()> 1 ? "A" : "B"             // No Memo 

but interestingly does not memo when using a getter ( 🐛 )
https://playground.solidjs.com/anonymous/6afa6081-5fa7-4ca6-b690-fcb92dbbe2e0

count()>1 ? props.a : props.b

@ryansolid is this intentional?

[ryansolid]

I think originally I was looking at this for components.. and since components were just function calls that's how I set the rule. To be fair it's a single option switch to have it wrap getters. I think it makes sense to wrap getters as well. This was probably overlooked initially and just stayed.