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

Add an option to aggressively opaque type #1121

Open
upsuper opened this issue Oct 30, 2017 · 11 comments
Open

Add an option to aggressively opaque type #1121

upsuper opened this issue Oct 30, 2017 · 11 comments

Comments

@upsuper
Copy link
Contributor

upsuper commented Oct 30, 2017

See Gecko bug 1412077, it may be a good idea to introduce a new option to aggressively mark types opaque based solely on the whitelist. This way, we can avoid generating types and their layout tests if we don't need them at all.

We can call the new option "whitelist-only", "aggressive-opaque", or something like that. And we probably should report error when this option is specified but there is still any opaque type in the list.

@upsuper
Copy link
Contributor Author

upsuper commented Oct 30, 2017

It would let us generate smaller binding files, and thus potentially accelerate building.

@emilio
Copy link
Contributor

emilio commented Oct 30, 2017

I doubt a lot the bottleneck of the build is that, fwiw. Since I had to investigate #1118, I have a single-file stylo input we can use to profile and figure out what is the bottleneck in bindgen.

Also, I doubt rust would struggle to handle a bunch of #[test] only free function definition.

@upsuper
Copy link
Contributor Author

upsuper commented Oct 30, 2017

This may not be a bottleneck for bindgen itself (although outputting fewer data might help a bit), but wouldn't generating more types give rustc more pressure when resolving them, especially when there are tons of useless types?

Also smaller binding files could also help analyzing and avoiding unrelated changes / conflicts when people have them in tree.

@emilio
Copy link
Contributor

emilio commented Oct 30, 2017

stylo-input.txt

This is the input, if someone wants to profile.

@emilio
Copy link
Contributor

emilio commented Oct 30, 2017

This may not be a bottleneck for bindgen itself (although outputting fewer data might help a bit), but wouldn't generating more types give rustc more pressure when resolving them, especially when there are tons of useless types?

I... guess? I don't know, I suspect this is not a big deal, but again without numbers, it's hard to say.

@upsuper
Copy link
Contributor Author

upsuper commented Oct 30, 2017

I... guess? I don't know, I suspect this is not a big deal, but again without numbers, it's hard to say.

I guess there is no way to verify this until we actually have such new option :)

@emilio
Copy link
Contributor

emilio commented Oct 30, 2017

Or until someone actually profiles the style crate and sees where rust spends its time :P

@upsuper
Copy link
Contributor Author

upsuper commented Oct 30, 2017

I think I saw some bug filed against rust before about having lots of types makes rustc take lots of time. But I failed to find that specific issue. That may have been resolved, though.

@upsuper
Copy link
Contributor Author

upsuper commented Oct 30, 2017

Oh that was about unused import. Never mind.

@tamird
Copy link
Contributor

tamird commented Nov 29, 2017

This would also be useful in cases where almost all types want to be opaque, but a select few require concrete representations.

It'd be really useful if there was a way to mark types opaque by default and then "clarify" the ones desired.

@fitzgen
Copy link
Member

fitzgen commented Nov 29, 2017

I could imagine something like

builder
    .opaque_type(".*")
    .no_opaque("SomethingWeCareAbout")
    .no_opaque("SomeOtherThing")
    // ...

which is analogous to derive_copy(true).no_copy("SomeSpecificType").

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

No branches or pull requests

4 participants