Update DOM Expression Libraries and Remove all hacks

[ryansolid]

With this PR I remove all direct DOM manipulation. No el.className = "danger" anywhere. No clever DOM node caching using reactive memorization. Not even shortcutting the test by dirtying the data model with a selected on each row. All my libraries will be doing an inline class={selected() === id ? "danger" : ""} or equivalent from now on.

By doing so all my libraries will take a performance hit. But the front end of this benchmark has gotten a bit ridiculous with bending the expectation. I hope with this I can encourage others to remove shady not in the spirit of the test implementation pieces. But if not at least they can serve as an example.

---

That being said I'm looking forward to testing a new technique I've developed. Inspired by Recoil's selectorFamily, I've developed a new reactive primitive for propagating change conditionally. It creates a keyed signal that only updates when the key matches without creating additional computations.

class={selected(id) ? "danger" : ""}

It is completely generic and has no knowledge of the DOM and can be leveraged like a normal signal in any data-binding or effect. It allows O(1) updates in these selection cases instead of O(n) and leverages the granularity to pull it off efficiently.

So far only Solid can leverage this technique though as I don't get the level of access I need in other libraries.

[trusktr]

It's really great to show the code that end users are most likely to (want to) write. The new code is cleaner and easier to read. The hacks were more verbose and not representative of what users want. 👍

[aminya]

Thanks for the change.

Your removed List implementation is still interesting, and it would be nice if it becomes a first-class API somewhere.
https://github.com/krausest/js-framework-benchmark/pull/794/files#diff-e21fa85f85f1b7a8cf7c1cd93a00f7dcL28

[ryansolid]

@aminya It used to be. Then I delegated it here and I finally now removed it. I thought that was the ideal solution at one point but while more optimal it is so specific here. Like what if there are dynamic children ie.. each row is a top-level conditional, or double nested arrays. There are just always places where the graph evaluates lazily so can't depend on full resolution at that point. This hooks in at the reactive phase but before we are connecting/diffing the real tree. My original original approach did a hook at the end of DOM rendering which worked better because everything was resolved.

Check out createSelector I replaced it with. It has none of those shortcomings. It is generic, not DOM specific just works off keys so can be nested and works with any binding. I finally solved it for real. It's just unfortunate my 3rd party libs don't give me enough access to their internals to make a version for each of them. I think I've finally found the right pattern here.

[krausest]

I think it's really a good move - thank you very much!
Here are the new results:

And for reference the old results:

[leeoniya]

@ryansolid quite impressive, 💯 !

[adamhaile]

👍 on removing "hacks." Since the PR talks about "all hacks" ... aren't there still a couple here? Should these have "Notes"?

      <For each={ state.data }>{ row => {
        const rowId = row.id;                                                                     // <-- [1]
        return <tr class={isSelected(rowId) ? "danger": ""}>
          <td class='col-md-1' textContent={ rowId } />                                           // <-- [2]
          <td class='col-md-4'><a onClick={[select, rowId]} textContent={ row.label } /></td>     // <-- [2]
          <td class='col-md-1'><a onClick={[remove, rowId]}><span class='glyphicon glyphicon-remove' aria-hidden="true" /></a></td>
          <td class='col-md-6'/>
        </tr>
      }}</For>

1. Hoisting the row.id dereference into a context where it won't create a dependency. For those not familiar with fine-grained reactive libraries, this line isn't just caching an intermediate value. If you replaced the locations in the code that reference rowId with row.id, then Solid's proxies would detect that dereference of a property during runtime and create a dependency link for each reference to row.id. That's non-trivial: it would increase Solid's bookkeeping costs by 50%, from 2 dependencies per row (isSelected and row.label) to 3 (those two plus row.id). So @ryansolid has lifted the dereference into a context -- the body of the For -- where Solid happens to have dereference detection turned off. He then uses the extracted value -- rowId -- in the contexts where dereference detection is turned on, thereby avoiding creating dependency links.
2. Setting the textContent property on the parent instead of a child text node, like <td class='col-md-1'>{ rowId }</td>.

2 feels like an optimization the Solid library should do internally -- Surplus used to, so I'm surprised Solid doesn't? Maybe it does and this is just vestigial?

1 bothers me a bit, as it's sort of a crypto-optimization. The code here "looks clean" but is actually doing a fairly advanced and impactful optimization. I think this is sort of a moral peril of the "looks clean" test -- it encourages submitters to hide the optimizations they're making. It would be better if submitters made it clear when they were doing optimizations vs a naive approach.

[ryansolid]

@adamhaile You are correct there are those 2 things going on. So maybe all hacks is wrong. I guess I still have library specific tricks. Just removed the general can do with any library direct DOM ones.

Hoisting prevents the compiler from incorrectly thinking the bindings are dynamic. I also have an explicit syntax for this using comments /*@once*/ but this approach also skips accessing the value more than once from the proxy.

And the second is that while I group all attribute updates in the same computation in a template block, inserts get their own. So using textContent is basically indicating the at the children are text nodes and lets it be considered cheap enough to be part of the same computation.

So those are 2 very subtle optimizations both related to the reactivity system and giving the compiler hints. The first is rather inconsequential for the pure signal version but does have a cost for the proxies as you pointed out. The second actually has considerable savings. Although it's really more like giving hints to the compiler similar to Inferno's $HasTextChildren:

<tr className={selected ? 'danger' : null}>
  <td className="col-md-1" $HasTextChildren>{id}</td>
  <td className="col-md-4">
    <a onClick={linkEvent(id, selectFunc)} $HasTextChildren>{label}</a>
  </td>
  <td className="col-md-1">
    <a onClick={linkEvent(id, deleteFunc)}>
      <span className="glyphicon glyphicon-remove" aria-hidden="true"/>
    </a>
  </td>
 <td className="col-md-6"/>
</tr>

Maybe there should be a note where the implementation uses compiler hints to optimize performance? Although it's pretty hard to crack down on. I did use internalization to hide the direct dom manipulation in the past and unless you looked closely you may have not noticed (I'm sure you did). I was happy to be rid of it as it wasn't generalizable.

When I write demos I tend to use my knowledge of the reactive system. So this isn't the only place you will see textContent or this sort of hoisting. I do this everywhere. If you remember Solid used to have an opt-in syntax for dynamic bindings. So while this is the reverse it us the patterns I promote to have that sort of control. Keeps in simple for those who don't know yet gives those with the knowledge the control I always craved from reactive libraries. While proxies are special here, I'm sure you leverage similar things to not call functions in bindings for the same reason (perhaps not in this benchmark). There is just as much no need to create computations everywhere someone accesses a member expression as it is for call expressions.

[adamhaile]

@ryansolid It's been a while, so I forgot that the textContent hack avoids creating extra computations.

That's a pretty significant (and very non-obvious) optimization.

How does Solid perform with a naive implementation, aka without the textContent and rowId optimizations?

For full disclosure, at one point Surplus had the same optimizations, but I later removed them, as they made me uncomfortable. Instead, I worked on optimizing no-dependency and single-dependency computation construction in S, so that I didn't need the hacks any more. (That work was the source1 stuff that I think you removed when you forked S.)

When we diverge from a naive approach for performance, I think we should make those differences explicit in the code. I don't know what Inferno's $HasTextChildren means (I've got a good guess), but I can see that and know that performance tuning has gone on, and that the perf numbers I'm seeing probably require expert-level knowledge of the library. I'd encourage you to put a comment on the rowId and textContent optimizations to make it clear why they're there.

I've said elsewhere that I'm not a fan of the flags idea, but if the benchmark does go down that path, seems like these are the kinds of things that should be noted. Maybe DOM manipulation for the textContent optimization? It's an uncommon way of interacting with the DOM b/c the framework encounters performance issues with the naive way. Maybe Update Tuning for Inferno's $HasTextChildren and the rowId optimization here?

I hear you that one of the things I like about real DOM, fine-grained libraries is that they put you in the middle of the process rather than outside it. Unfortunately, I think that battle is lost. Clearest sign is that libraries which break the DOM's event bubbling model for performance are called clean, while those that work with event bubbling get flagged.

[ryansolid]

Writing a comment seems reasonable.

I personally put this all in the same category as what Inferno is doing. Only because it is non-obvious that it gives more information to the compiler to produce more optimal code. It's not like it escapes the declarative syntax. When the expression is dynamic it updates like anything else. It is a way of indicating the children are only text which I thought was pretty nice in that it doesn't introduce new syntax. Most libraries with JSX have a way of setting innerHTML as well. Same sort of thing. It is a declarative prop/attribute abstraction on a DOM API but that is what most data-bindings are.

I think the event delegation is a losing battle. But I really see no problem with it as it is the way all libaries solve the problem internal or externally. Official docs for libraries like Knockout, and Vue direct you to doing explicit event delegation. This is the defacto way people approach solving this so I don't know. I have included event delegation internally for ease but I don't think that is a choice that needs to be made by the library.