svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Size regression from Svelte 4

Open benmccann opened this issue 1 year ago • 5 comments

Describe the bug

My site at https://c3.ventures/ actually grows in size when migrating from 4.2.18 to 5.0.0-next.233.

Svelte 4: chunks - 38,059 bytes entry - 6,079 bytes nodes - 61,245 bytes

Svelte 5: chunks - 47,499 bytes entry - 5,255 bytes nodes - 61,206 bytes

I believe this is primarily triggered in my case by the site having 25 instances of @sveltejs/enhanced-img though I'm not sure that's necessary to hit this. Inlining the srcset during template creation would reduce the HTML creation portion of the compiled JS from 21K to 6.8K (this is not the whole script - i.e. it excludes Svelte's runtime, etc.)

It's possible that solution to this might be https://github.com/sveltejs/svelte/issues/11843, but I'm not 100% sure so have filed this as a separate issue to not confuse that thread in the case that it isn't the way to solve this

Reproduction

Here's a REPL with 25 picture tags that generates over 600 lines of output. It should probably be able to be done in just a few lines.

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA-2XwWqDQBBAf2UZCmlB3BTSlAQN9N5ToaduD1bHuKBG3DVSxH8v29BbSi8lMJO5LCoOvOU9FnaC0tboYPs2QZs1CFt46jqIwH924cUdsfYIEbjD0OfhS1LY4-7s0tncDz3uTGt8cvpfuT536NPJwM3U4qheX55vF3GsM-fQO_0QZ0dbLiJlm-7Q-7hBn8VDX9_FVY_lrO5Xq-UYqbOz6z9mN-vlaGBWYSepAdtke9RhxIDSAvm_kLbZB8LUgH40oLLapwY-Mmdz5dF5A2q0ha_SgKEqtPvKn57DfKJ_0kn0d0m_rKaV-ARS4rsWryQgJT6eXklASnw8vZKAlPh4eiUBKfHx9EoCUuLj6ZUEpMTH0ysJSImPp1cSkBeKT_ITSDn7rscrCUiJj6dXEpASH0-vJCAlPp5eSUDKlYOrWRKQcvbx9EoCUuLj6ZUEpMTH0ysJSImPp1cSkHLl4GqWBKScfTy9koCU-Hh6JQEp8fH0SgJS4uPplQTkxa4cpoUImkNhS4sFbH0_4Pw-fwGCAfUUhT4AAA==

Logs

No response

System Info

5.0.0-next.233

Severity

blocking an upgrade

benmccann avatar Aug 23 '24 18:08 benmccann

Is it even possible to inline srcset if it requires the execution of JS new URL(...).href? Shouldn't here the asset import be used instead?

<script>
import img5 from "../assets/5.avif";
import img6 from "../assets/6.avif";
</script>

<source srcset="{img5} 1440w, {img6} 960w" type="image/avif" />

It even looks much better.

7nik avatar Aug 23 '24 23:08 7nik

Is it even possible to inline srcset if it requires the execution of JS new URL(...).href?

Yes

Shouldn't here the asset import be used instead?

This isn't code being written by hand. The user writes <enhanced:img src="./path/to/your/image.jpg" alt="An alt text" />. This is the output of a resolved image URL as returned by Vite in the preprocessor.

benmccann avatar Aug 23 '24 23:08 benmccann

Looked into this but it's tricky. We can't just hoist every expression that doesn't contain a reference to an instance-level binding, because it's reasonable to expect that <p>{location.href}</p> or <p>{Date.now()}</p> would evaluate when the component is rendered, rather than when the module first evaluates.

Svelte 4 gets away with this by doing element.innerHTML = ... for static subtrees. Svelte 5 instead uses template cloning for performance reasons. We could maybe do innerHTML for static-after-render subtrees, but it would be quite a substantial change, and given that this is non-critical I'm going to move it off the 5.0 milestone.

In the meantime, it would be good to have a better repro. This one is wrong. You have this repeated...

<div><div><div><div><div><picture>
  <source srcset={"${new URL('../assets/5.avif', import.meta.url).href} 1440w, ${new URL('../assets/6.avif', import.meta.url).href} 960w"} type="image/avif" />
  <source srcset={"${new URL('../assets/5.avif', import.meta.url).href} 1440w, ${new URL('../assets/6.avif', import.meta.url).href} 960w"} type="image/avif" />
  <source srcset={"${new URL('../assets/5.avif', import.meta.url).href} 1440w, ${new URL('../assets/6.avif', import.meta.url).href} 960w"} type="image/avif" />
  <img src="/7" alt="basic test" width=1440 height=1440 />
</picture></div></div></div></div></div>

...when I assume what you mean is this:

<div><div><div><div><div><picture>
  <source srcset="{new URL('../assets/5.avif', import.meta.url).href} 1440w, {new URL('../assets/6.avif', import.meta.url).href} 960w" type="image/avif" />
  <source srcset="{new URL('../assets/5.avif', import.meta.url).href} 1440w, {new URL('../assets/6.avif', import.meta.url).href} 960w" type="image/avif" />
  <source srcset="{new URL('../assets/5.avif', import.meta.url).href} 1440w, {new URL('../assets/6.avif', import.meta.url).href} 960w" type="image/avif" />
  <img src="/7" alt="basic test" width=1440 height=1440 />
</picture></div></div></div></div></div>

The first test weighs 2399 bytes after gzip in Svelte 5 and only 1301 in Svelte 4, but when using the corrected version the output weighs 2369 bytes in Svelte 5 and 6189 bytes in Svelte 4. So unless enhanced-img is generating borked markup (which seems unlikely), then there's probably something else going on in your app to result in this outcome.

Rich-Harris avatar Aug 27 '24 17:08 Rich-Harris

If we can't hoist in the general case, I could still update enhanced-img to manually hoist these expressions since I know it is safe in that specific case. However, it seems even when manually hoisted they're still not inlined:

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA42SXWvCMBSG_0o4txbqR3XMtULZyuaF3ii7WcahpmkN9CM0qUWk_33ETuaFC705nIQ8ecJ5c4FU5FzB8usCZVxwWEIoJTigz9Is1InnmoMDqmpqZnZ8xWohNSmqpMn5ipZUs6pUmiBG249w-xq94XrzjhNEEhAKiJ_rfYThbhftEafh5gnPGCNSePmXndrYg52d2VhmZz0bm9jZuY3ldnZhY9Nf1nf7yZuRl34iTqu7IgXTTd3H4fdZEVUzxXVweRDMiFAgE88bt47pRo8CuJ55XoxbCh0x3yGgIIo44258EikF4g6xzQbYPKut5Qc51DYfYFtYbbLM_mSiyIzpkaYjca4DCrKukoZpUZVEc6UpkFYk-hiYB5AjF9lR97250ndvOfnuNba7SktwoKgSkQqewFLXDe--ux_WbPJ3oAMAAA==

Any chance we could add this to the 5.0 milestone with the smaller enhancement request of inlining expressions from <script module> blocks? I could update enhanced-img before 5.0 as well then.

benmccann avatar Aug 27 '24 18:08 benmccann

Yeah, that seems achievable. I'd still like to understand why you're seeing larger output with 5 than 4 though — that's just not what the respective REPLs are telling us. It has to be something else

Rich-Harris avatar Aug 27 '24 21:08 Rich-Harris

Decided that the juice isn't worth the squeeze for 5.0, especially when the repro is questionable (significantly larger in 4 than in 5) – moving it back out to 5.x. We need to ship

Rich-Harris avatar Aug 29 '24 14:08 Rich-Harris

Another more general possibility for static subtrees: We could do template(() => '<img src=${new URL("stuff")}') (note the additional lambda function) if we detect that the things in question only reference global/module scope things. Instead of receiving and caching the result, the template would be recloned every time (possibly even taking a different route, like .innerHTML = ...). That way it becomes smaller in more cases.

dummdidumm avatar Sep 13 '24 15:09 dummdidumm

This is solved with the latest release (numbers differ from above because I've made other changes to my site)

4.2.19 chunks - 39,785 bytes entry - 6,144 bytes nodes - 70,071 bytes immutable - 1,273,979 bytes

5.0.0-next.30 chunks - 51,879 bytes entry - 4,545 bytes nodes - 47,211 bytes immutable - 1,257,491 bytes

5.0.0-next.245 chunks - 44,334 bytes entry - 8,782 bytes nodes - 53,348 bytes immutable - 1,258,972 bytes

benmccann avatar Sep 13 '24 16:09 benmccann

Another more general possibility for static subtrees: We could do template(() => '<img src=${new URL("stuff")}') (note the additional lambda function) if we detect that the things in question only reference global/module scope things. Instead of receiving and caching the result, the template would be recloned every time (possibly even taking a different route, like .innerHTML = ...). That way it becomes smaller in more cases.

That would kill performance. We need to clone the template once, otherwise templates become irrelevant.

trueadm avatar Sep 13 '24 19:09 trueadm

I'm not talking about doing it in the general case, only when we detect there's static-but-possibly-different-between-instances variables referenced. Doing innerHTML once instead of clone + walk + mutate might actually be faster

dummdidumm avatar Sep 13 '24 20:09 dummdidumm

I'm not talking about doing it in the general case, only when we detect there's static-but-possibly-different-between-instances variables referenced. Doing innerHTML once instead of clone + walk + mutate might actually be faster

Let's benchmark it and see then. It might be for these cases, but it's theory at this point and a good investigation.

trueadm avatar Sep 13 '24 20:09 trueadm

Tested it, sadly innerHTML is still slower compared to cloning + walking the tree. Though I realized through that benchmark that you're only benefitting from the clone approach when you're using a component more than once, otherwise it's strictly slower.

dummdidumm avatar Sep 18 '24 09:09 dummdidumm