-
Notifications
You must be signed in to change notification settings - Fork 187
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
Make Debug impls optional #482
Conversation
8db8809
to
72bf38e
Compare
I wonder should we list available features in the readme so people know the built time optimizations exist. |
72bf38e
to
93c527f
Compare
Since nobody seems to feel strongly, I've updated this to disable the feature by default. |
I'm tracking the windows-rs repository, and users seem rather disgruntled by the removal of |
723ef6b
to
37569f3
Compare
I don't have enough experience with winapi to judge how the usage patterns might vary. I'd rather have this in but enabled by default than not in at all, if that's your preference? I'll ask around and see if anyone I know would be impacted; hard to predict in general. |
OTOH I'm not sure how useful our limited debugging implementation is: we don't even (and for good reason) dereference and follow pointers. Still, at least being able to see pointer values and everything else that's not a pointer already goes a long way. Still on the fence about default-enabling or disabling it... |
37569f3
to
e743000
Compare
I think our current Given the uncertainty, I think it's better for us to merge this with the feature enabled by default than agonize over it, so I've turned it back on. We can always disable in the future. |
e743000
to
9a93ef2
Compare
@Ralith On my machine I can win another 400-500ms (7.9s down to 7.4s) by removing We may want to preserve just the debug for |
9a93ef2
to
5eacc92
Compare
I preserved |
5eacc92
to
0a797d4
Compare
When disabled, this buys us a 12% reduction in buildtime.
0a797d4
to
2e8f0ec
Compare
e6b244f
to
13bf0c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
(I hope nobody notices the cfg_attr
being below instead of above the derive
s like in vk/definition.rs
- I did 😬)
When disabled, this buys us a ~12% reduction in buildtime in my environment
I'd like some discussion on whether it should be enabled by default. It's less breaking that way, but I dunno how often people are going to scan the feature list for opportunities to improve buildtime.