sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.21k stars 4.09k forks source link

Size regression from Svelte 4 #12995

Open benmccann opened 2 weeks ago

benmccann commented 2 weeks ago

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

7nik commented 2 weeks ago

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.

benmccann commented 2 weeks ago

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.

Rich-Harris commented 2 weeks ago

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.

benmccann commented 2 weeks ago

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.

Rich-Harris commented 2 weeks ago

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 commented 2 weeks ago

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