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.

Sign up for GitHub

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

Jump to bottom

Code bloat from multiple functions being generic over every closure #234

Closed
anp opened this issue Jan 4, 2021 · 3 comments · Fixed by #248
Closed

Code bloat from multiple functions being generic over every closure #234

anp opened this issue Jan 4, 2021 · 3 comments · Fixed by #248
Labels
illicit Related to `illicit` crate for implicit context topo Related to the `topo` crate for incremental callgraphs
Milestone
topo 1.0

Comments

@anp
Copy link
Owner

anp commented Jan 4, 2021

In @zetanumbers' project, topo::call's generic instantiations take up over 20% of final binary size:

topo-code-bloat

I made it into a function as part of using #[track_caller] for everything, but this is obviously causing problems.

My initial thought was that we could make the function take a trait object reference, but that won't work because we take an FnOnce by-value.

I think it should be possible to make topo::{call,call_in_slot} back into a single macro that optionally accepts a slot for the call. Because it still uses track_caller it won't change the interfaces of any code "above" topo in the callstack, and because most code now uses #[topo::nested] the change shouldn't even result in a very large diff. It will still be a breaking change, though.
@anp anp added the topo Related to the `topo` crate for incremental callgraphs label Jan 4, 2021
@anp anp added this to the topo 1.0 milestone Jan 4, 2021
@anp
Copy link
Owner Author

anp commented Jan 4, 2021

One downside to this is that moving to functions allowed me to remove some technically-private-but-unstable machinery from the crate. #[docs(hidden)] it'll have to be.

@anp anp changed the title Code bloat from topo::call() being generic over every closure Code bloat from multiple functions being generic over every closure Jan 4, 2021
@anp
Copy link
Owner Author

anp commented Jan 4, 2021

As pointed out on discord in earlier discussion, https://docs.rs/illicit/1.1.1/illicit/struct.Layer.html#method.enter is also generic and also takes the closure by-value. I think illicit::Layer::enter is probably the real source of bloat here, but could be wrong.

@anp anp added the illicit Related to `illicit` crate for implicit context label Jan 4, 2021
@anp
Copy link
Owner Author

anp commented Jan 4, 2021

From discussion on discord, current plan is to change illicit::Layer::enter to return a guard instead of taking a closure. In topo's internals we'll also need to rework things so that Scope::with_current/Scope::enter_child are no longer generic functions that vary between closure types.

If after that there's still significant bloat from topo::call, we'll try adding #[inline(always)] to it and call_in_slot, which should remove the remaining function stubs.

I'm filing a separate issue about regression tests for this so that we can keep from getting back into this scenario.

@anp anp mentioned this issue Jan 4, 2021
Open
anp added a commit that referenced this issue Feb 2, 2021
@anp
[topo][illicit] Reduce binary size of topo users.
bfe94ab 
Fixes #234.

Steps taken:

1. Outlines the non-generic portion of the illicit::Layer::enter
   function body, adds inline(never) to the outlined function.
2. Reduce genericity of call internals, returning a Layer rather than
   entering it.
3. Strategic inlining for binary size. Some generic functions get called
   so much that their main contribution to bloat is their preludes.
   Inlining them allows for a more compact representation akin to
   how macros look when expanded.

TodoMVC size before: 691604
TodoMVC size after: 579014
Size savings: ~16.3%

After this change the twiggy output looks mostly limited to dyn-cache
and event callbacks:

 ~Bloat % │ Monomorphizations
─-────────┼─────────────────────────────────────────────────────────────
    4.77% ┊ hashbrown::raw::RawTable<T>::reserve_rehash
    3.30% ┊ dyn_cache::local::SharedLocalCache::cache
    3.00% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::store
    2.00% ┊ augdom::webdom::Callback::new::{{closure}}
    1.84% ┊ dyn_cache::dep_node::Dependent::init_dependency
    1.58% ┊ dyn_cache::local::SharedLocalCache::cache_with
    1.39% ┊ illicit::Layer::offer
    1.25% ┊ core::ptr::drop_in_place
    1.05% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::get
    0.95% ┊ moxie_dom::interfaces::element::ElementBuilder::class
   16.69% ┊ ... and 619 more.
   37.83% ┊ Σ [629 Total Rows]
@anp anp mentioned this issue Feb 2, 2021
Merged
anp added a commit that referenced this issue Feb 2, 2021
@anp
[topo][illicit] Reduce binary size of topo users.
3a3113c 
Fixes #234.

Steps taken:

1. Outlines the non-generic portion of the illicit::Layer::enter
   function body, adds inline(never) to the outlined function.
2. Reduce genericity of call internals, returning a Layer rather than
   entering it.
3. Strategic inlining for binary size. Some generic functions get called
   so much that their main contribution to bloat is their preludes.
   Inlining them allows for a more compact representation akin to
   how macros look when expanded.

TodoMVC size before: 691604
TodoMVC size after: 579014
Size savings: ~16.3%

After this change the twiggy output looks mostly limited to dyn-cache
and event callbacks:

 ~Bloat % │ Monomorphizations
─-────────┼─────────────────────────────────────────────────────────────
    4.77% ┊ hashbrown::raw::RawTable<T>::reserve_rehash
    3.30% ┊ dyn_cache::local::SharedLocalCache::cache
    3.00% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::store
    2.00% ┊ augdom::webdom::Callback::new::{{closure}}
    1.84% ┊ dyn_cache::dep_node::Dependent::init_dependency
    1.58% ┊ dyn_cache::local::SharedLocalCache::cache_with
    1.39% ┊ illicit::Layer::offer
    1.25% ┊ core::ptr::drop_in_place
    1.05% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::get
    0.95% ┊ moxie_dom::interfaces::element::ElementBuilder::class
   16.69% ┊ ... and 619 more.
   37.83% ┊ Σ [629 Total Rows]
@anp anp closed this as completed in #248 Feb 2, 2021
anp added a commit that referenced this issue Feb 2, 2021
@anp
[topo][illicit] Reduce binary size of topo users.
9ecadba 
Fixes #234.

Steps taken:

1. Outlines the non-generic portion of the illicit::Layer::enter
   function body, adds inline(never) to the outlined function.
2. Reduce genericity of call internals, returning a Layer rather than
   entering it.
3. Strategic inlining for binary size. Some generic functions get called
   so much that their main contribution to bloat is their preludes.
   Inlining them allows for a more compact representation akin to
   how macros look when expanded.

TodoMVC size before: 691604
TodoMVC size after: 579014
Size savings: ~16.3%

After this change the twiggy output looks mostly limited to dyn-cache
and event callbacks:

 ~Bloat % │ Monomorphizations
─-────────┼─────────────────────────────────────────────────────────────
    4.77% ┊ hashbrown::raw::RawTable<T>::reserve_rehash
    3.30% ┊ dyn_cache::local::SharedLocalCache::cache
    3.00% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::store
    2.00% ┊ augdom::webdom::Callback::new::{{closure}}
    1.84% ┊ dyn_cache::dep_node::Dependent::init_dependency
    1.58% ┊ dyn_cache::local::SharedLocalCache::cache_with
    1.39% ┊ illicit::Layer::offer
    1.25% ┊ core::ptr::drop_in_place
    1.05% ┊ dyn_cache::namespace::Namespace<Scope,Input,Output,H>::get
    0.95% ┊ moxie_dom::interfaces::element::ElementBuilder::class
   16.69% ┊ ... and 619 more.
   37.83% ┊ Σ [629 Total Rows]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
illicit Related to `illicit` crate for implicit context topo Related to the `topo` crate for incremental callgraphs
Projects
None yet
Milestone
topo 1.0
Development

Successfully merging a pull request may close this issue.

[topo][illicit] Reduce binary size of topo users. anp/moxie
1 participant
@anp