Skip to content

Conversation

@tiagoshibata
Copy link
Contributor

@tiagoshibata tiagoshibata commented Jun 25, 2020

Description: Supporting disabling RTTI and other options for reducing binary size were requested by customers.

The only more relevant change was the inheritance around IControlFlowKernel to avoid casting sideways in the inheritance tree. Previously, control flow kernels would inherit from OpKernel and IControlFlowKernel. Since all IControlFlowKernel were also OpKernel (and no class inherited from IControlFlowKernel only), I made IControlFlowKernel inherit from OpKernel, so a simple downcast/upcast without RTTI can be done.

I believe all breaking changes are restricted to the CPU provider and don't affect any public APIs. Let me know if I'm wrong and we can discuss alternatives.

@tiagoshibata tiagoshibata requested a review from a team as a code owner June 25, 2020 00:49
@pranavsharma
Copy link
Contributor

@tiagoshibata can you please quantify the reduction in size after these changes? thx.

@pranavsharma
Copy link
Contributor

Should we do this for ONNX as well? ONNX might not be using RTTI now, but having a compile flag will avoid the usage going undetected.

@snnn
Copy link
Contributor

snnn commented Jun 25, 2020

Just wondering: what if in a single PE file, part of the code was compiled with RTTI, part of it wasn't. Would it cause problem at link time?

@tiagoshibata
Copy link
Contributor Author

@pranavsharma 436K - about the same that I reported before

@snnn good question. I believe that, if there's a class with virtual members, and one compilation unit with RTTI disabled and another one with it enabled both use it, there will be a linker error or runtime problems

skottmckay
skottmckay previously approved these changes Jun 26, 2020
@tiagoshibata
Copy link
Contributor Author

I rebased to fix a conflict in the root CMakeLists.txt. Can you take a look again? Thanks!

@tiagoshibata tiagoshibata force-pushed the user/ticastro/no-rtti-build branch from 1fe335c to 6daf7cb Compare July 1, 2020 16:37
@tiagoshibata
Copy link
Contributor Author

Rebased again due to Mac pipeline being misconfigured in master. Hopefully this passes now.

@tiagoshibata tiagoshibata merged commit 7fea332 into master Jul 1, 2020
@tiagoshibata tiagoshibata deleted the user/ticastro/no-rtti-build branch July 1, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants