-
-
Notifications
You must be signed in to change notification settings - Fork 705
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
Conversation
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 |
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 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. |
@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. 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:
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: |
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. |
One point of clarification: The bug that we are talking about is, specifically, the fact that attribute spreading onto components that return 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! |
It's no problem, I understand the frustration! I've felt similar trying to solve it 😅 .
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 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 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 Some other things iirc i've attempted:
Some of it helps with compile time, but nothing big enough to solve the types issue. Happy to discuss offline too |
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.