Idea: create more functional code for fragments

[Swatinem]

One thing that actually bothers me when looking at the svelte output is that it uses closures for the update and teardown methods, which might be ok for minification, because it can minify all the variable names, but is actually bad for code reuse inside the VM.
I also experimented with a similar code generation idea over a year ago, and I thought to myself, why not make those things functional, like this:

• Fragment.create(…) => Instance appends to the DOM and returns an object with the important dom elements
• Fragment.update(Instance, …) instead of using closure upvars or this, we just pass in the object with the dom elements
• Fragment.teardown(Instance) same here

Also, thinking about #1 and #9, we could also have these methods:

• Fragment.toString(…) will render to a string for SSR
• Fragment.hidrate(…) => Instance will create the element map from already existing DOM elements

I think this can be done internally without changing the public API of components.

What do you think @Rich-Harris?

[Rich-Harris]

Anything that makes the VM's job easier is 👍 . I'd be curious to know how much of a benefit there is in moving update and teardown out of the closure though – since you still have to keep the fragment in memory, it's just in an object rather than in the closure. Will confess I'm not totally sure how these things work!

I do think it's worth striving for readable generated code – one of the nice things about a non-runtime approach is that it's beautifully easy to debug since you're stepping through ~150 lines of code rather than ~20,000 lines of framework. Keeping update and teardown in the closure helps there. Though clearly performance is a higher priority.

For SSR I was thinking you'd keep that out of the main compiler altogether, and it would be a totally separate thing (well, same parser, but different generator). Could even be a streaming renderer which would be pretty neat. Not totally sure yet what hydration looks like...

[mrkishi]

I think this is something worth exploring, especially since closures make a big difference in V8.

@Rich-Harris I highly recommend reading Vyacheslav Egorov's Grokking V8 closures for fun (and profit?) if you're interested in closures' performance characteristics. I reached out to them and they confirmed that post is still 100% relevant in today's V8.

I'll try to run some benchmarks on an object-based solution when I get a chance.

[Swatinem]

Some hard numbers would sure be nice. I know that it should in theory be better for the vm, surely from a memory usage perspective, since it avoids creating a lot of duplicated function objects, but I have no idea what this means in practice.

Anyway, here is a completely outdated branch where I tried a little bit to do this: https://github.com/sveltejs/svelte/commits/functional
The branch would need quite some changes, but the recent work towards create/mount is a step in the right direction already.

[mrkishi]

Instead of hacking on the compiler, I decided to first create a simple benchmark by manually editing a generated component.

I did a direct closure to object translation instead of making big changes so that we can compare the impact of closures by themselves. Implementing these changes to the compiler shouldn't be too hard as there's close to no distinction.

You can find the benchmarks here.

On my machine, closures are nearly twice as slow on Chrome, but there's no significant difference on Firefox (although they're generally a few percent points slower).

I realize this is a pretty constrained and silly benchmark, but we're talking about a fairly low-level vm optimization, here. I have no reason to believe closures would outperform objects given a realistic use case.

A non-closure implementation would also give us more control over the general dependencies/memory layout, so I expect it'd be easier to implement future optimizations.

I personally think V8 is too big of a target for us to neglect "those gains."

Thoughts? @Swatinem @Rich-Harris

[FWeinb]

I did modify the generator to compile into a more object style but the results doesn't look too promesing. There are some gains but I expected it to be much better:

Here are the results of the js-framework-benchmark. svelte v1.0.7-test is using the object approche.

[mrkishi]

@FWeinb I did the same and got surprisingly close results. 😄

I did it naively at first, adding all references to the object and without extracting event handler closures. Got from 1.12 -> 1.10.

I was about to start adding support for init-only references (without adding them to the instance), but if you're working on the same thing I might as well check if you're further along.

Did you implement the distinction between instance members and init-only variables? What about event handlers and getBlocks, did you extract them already?

[FWeinb]

I just pushed the changes I made to a branch on my fork. You can find it here I did move every element/node creation to this and than added the render/mount/update methods to the prototype.

[mrkishi]

Talk about some duplicated effort... 😋

[FWeinb]

Okay. We had the same idea! Funny that is looks almost identically.

[Swatinem]

Nice work. I’m personally in favor of not using this but passing in the instance explicitly. That way, you can have one function to create an instance from scratch and one to hidrate an instance from already existing DOM (from SSR for example, #1). And also a static function to render to a string.

I am personally in favor of also splitting up the state management from the dom manipulation ( #9 ), and ideally, the dom manipulation part would just export a module with static functions for everything, and a tool like rollup could be used to DCE away things you don’t need, like the create, update, teardown part in case of SSR, or the hidrate, toString part in you don’t use SSR.

But that is a quite invasive change :-)

[mrkishi]

Yeah, I tried to do a conversion as closely as possible, at first.

I'm still wrapping my head around the generator. I don't even know how all generator.current properties work, yet. For instance, I had a problem with RawMustacheTag when rebasing those changes at mrkishi/svelte@266e894 because it uses generator.contextualise's snippet... It goes without saying this is just an exploration at this point.

For us to really add this to svelte, I think it'd be wise to track identifiers/references scopes/owners as well, instead of adding this. or instance. willy-nilly to the codegen. It gets fragile.

[benmccann]

I believe this ticket is specific to Svelte 2, so I'm going to close this. If that's wrong we can reopen - though it may be cleaner to file a new issue with Svelte 3 code examples and consolidated feedback from this ticket in that case