Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Size regression from Svelte 4 #12995

Closed
benmccann opened this issue Aug 23, 2024 · 12 comments
Closed

Size regression from Svelte 4 #12995

benmccann opened this issue Aug 23, 2024 · 12 comments
Milestone

Comments

@benmccann
Copy link
Member

benmccann commented Aug 23, 2024

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 #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 benmccann added this to the 5.0 milestone Aug 23, 2024
@7nik
Copy link

7nik commented Aug 23, 2024

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
Copy link
Member Author

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
Copy link
Member

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 Rich-Harris modified the milestones: 5.0, 5.x Aug 27, 2024
@benmccann
Copy link
Member Author

benmccann commented Aug 27, 2024

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
Copy link
Member

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
Copy link
Member

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

@dummdidumm
Copy link
Member

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.

@benmccann
Copy link
Member Author

benmccann commented Sep 13, 2024

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

@trueadm
Copy link
Contributor

trueadm commented Sep 13, 2024

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.

@dummdidumm
Copy link
Member

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

@trueadm
Copy link
Contributor

trueadm commented Sep 13, 2024

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.

@dummdidumm
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants