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

AddAnyAttr for AnyView for non-erased #3553

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

zakstucke
Copy link
Contributor

@zakstucke zakstucke commented Feb 6, 2025

Reattempt of #3548 on 0.8 branch, made a hash of switching branches.

For AddAnyAttr in non-erased mode, the issue is simply the number/size of the types generated once something has already become an AnyView being too much for the compiler to handle.

This PR "truly erases" AnyView by preventing further types being created once something's been erased, utilising an "attribute bag" that will be passed down through the rendering traits, instead of producing a new type each time an attribute is added.

It's definitely the brute-force fix, but it seems to work!

Edit:
The into_owned() method being added to RenderHtml is somewhat separate, but is neccessary to make erased mode not cause new compile errors relating to AnyView not being static, it allows removing the static bound from IntoAny.

@zakstucke zakstucke changed the base branch from main to leptos_0.8 February 6, 2025 21:34
@benwis
Copy link
Contributor

benwis commented Feb 6, 2025

Awesome work as always, I see no harm in merging this one for experimentation purposes. I'll ping @gbj here in case he wants to chat, but let's try it

@benwis benwis merged commit 8f74a6d into leptos-rs:leptos_0.8 Feb 6, 2025
@gbj
Copy link
Collaborator

gbj commented Feb 7, 2025

I have not yet had time to look at this in detail, but the implementation involved passing type-erased attributes, or the potential for type-erased attributes, everywhere.

A quick test against the hackernews example shows a 426kb release build before this commit, and 454kb afterwards, i.e., this adds 18% to the binary size of the application. For todomvc, it's 210kb to 216kb. For axum_js_ssr, it's 617kb to 661kb.

To me, this is prohibitive.

Making type erasure more pervasive will have this bloating affect and should be avoided if possible: the reason the view tree is typed so thoroughly is to allow the compiler to remove dead code, whereas the type-erased types end up adding significant amounts of unused code.

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 7, 2025

@gbj I'm not sure if I'm missing something, but those changes sound like 6.6%, 2.9% and 7.1% respectively?

It's important to remember that this could literally be a side effect of fixing the bug: there are now attributes included that were previously being thrown away. AnyView is used internally in Fragment, ChooseView and the routers for some sections, on top of any direct downstream usage.

I haven't spent anytime looking at binary size, I'll do so this weekend. But even if that doesn't lead to anything, I don't think it takes away from this PR being a good idea right now in that:

  • This is a silent but important bug, that has already been noticed downstream multiple times.
  • This provides parity regarding attribute spreading with erase_components, making fast compiles a first-class citizen
  • There is as of now no alternative, I've spent quite some time playing around with the issue and the compiler: it's simply just too many types. As it stands I don't see any alternative method to fix the bug.
  • The binary size change isn't drastic, and may be explained away by the "missing code", i.e. the lack of attributes that should've existed but were thrown away prior to this. In my opinion, a serious bug trumps a binary size change (if no alternative way of fixing the bug).
  • It's easily reversed in a 0.9 if a better solution arises, or rust / compiler improves performance, the latter I don't hold out much hope for. The breaking change on the average downstream user will be next to none now or in a reversal.

How are you benchmarking binary size? Just so we're on the same page. I'm currently testing with the below, but getting quite different results:
cargo +stable leptos build --release then checking target/site/pkg/$name.wasm

@gbj
Copy link
Collaborator

gbj commented Feb 7, 2025

Sure, I just can't do math. Still, those are pretty large changes from my perspective.

I do not think that the changes are due to including attributes that were previously thrown away: in the examples I compared, I haven't noticed that there are elements missing attributes that they should have. If you have noticed missing attributes in those three examples that this fixes, could you point them out to me? I would be glad to fix that.

Like I said, I have not reviewed this PR other than taking a glance and testing the size increases, so I don't have any further substantive thoughts.

@gbj
Copy link
Collaborator

gbj commented Feb 7, 2025

This is a silent but important bug

One point of clarification: The bug that we are talking about is, specifically, the fact that attribute spreading onto components that return AnyView doesn't work, right? I just want to make sure I'm on the same page.

I always want to rewind a minute and start where I should've started: Thanks so much for your work on this and related issues trying to fix these very annoying and difficult compile-time/type issues. I think I've been a little abrupt in my responses at points, and I want to acknowledge that that's nothing to do with you, and entirely to do with my frustration with the fact that something that the language seems to support and that works at all the scales that I have regular access to test then breaks, in vexatious ways, at larger scales... and that I only discovered it at the end of a long and fairly exhausting process of building all this out.

So: apologies, and thanks.

I think the best course of action is going to be for me to dedicate some significant chunk of time to trying to understand the "too many types" problem, which I'll admit I still have not grokked, and then trying to understand how these solutions fix it, and whether there's a way to do it without the additional overhead. I just do not feel confident that I understand the problem or am able to maintain the solution. I haven't had much headspace to do deeper work like this on the framework recently, but I will make some time when I am able to. I think that will make me feel better about it!

@zakstucke
Copy link
Contributor Author

zakstucke commented Feb 7, 2025

It's no problem, I understand the frustration! I've felt similar trying to solve it 😅 .

One point of clarification: The bug that we are talking about is, specifically, the fact that attribute spreading onto components that return AnyView doesn't work, right? I just want to make sure I'm on the same page.

Yes. #3518 provided a fix for the erased branch, making erased work perfectly as a faster-to-compile version of leptos for dev work (currently about 50% faster, I have a few more PR's lined up that make it another 50% faster on-top). It was a much bigger problem on there, because everything becomes an AnyView so all attributes would be lost. But this left "stable" leptos in a sticky situation of now having a "bug" that erased does not: the anyviews that do exist do not respect their attributes, albeit far less important now it works on erased but a confusing inconsistency nontheless. This PR is an attempt at fixing the issue on "stable", which does not require vectorising everything and the imagined performance issues that would entail (I actually haven't tested erased yet for binary size or runtime speed, but it definitely runs fine!).

I'm not a big fan of the solution either, but I've been quite deep down the rabbit hole and this is the only way I've gotten it working without vectorising things that weren't intended to be AnyView's, which I think would have bigger downsides. Both the vectorising fix and this one, both exemplify and solve the root issue: too large / many types if they aren't culled at each AnyView boundary (the intended usage of AddAnyAttr produces a brand new type each time, it doesn't matter AnyView is erased, it still produces a new internal T).

The best tool I've been using is cargo-llvm-lines, it's somewhat limited but it really shows when things blow up - e.g. iirc it's possible to compile with AddAnyAttr as originally intended without this fix if you gut AnyView of all it's contents (todo!()'s everywhere), but you then get to see that simply by adding the implementation, the llvm lines shoot up from about 10million to 100million.

Some other things iirc i've attempted:

  • Optimising hot trait implementations, which is where all the monomorphisation bottlenecks are that produce too much compiler code
  • Removing const generics and replacing them with functions, which supposedly helps
  • dynamic dispatch as an alternative to monorphised erasure

Some of it helps with compile time, but nothing big enough to solve the types issue.

Happy to discuss offline too

This was referenced Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants