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

Fix server-side fragmentation #1239

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

j18ter
Copy link
Contributor

@j18ter j18ter commented Sep 12, 2020

This fixes issue #1201.

The existing implementation attempted to implement fragmentation in the
TAO_OutputCDR inserters, but this did not work because array
and sequence types, including strings, were implemented at
the ACE_OutputCDR layer, which remained oblivious to fragmentation.
The old implementation also left TAO_OutputCDR with a dangerous
interface. The write_xxx functions inherited from ACE_OutputCDR did
not fragment, only the inserters did.

The proposed implementation fragments in the low-level write_1, write_2,
write_4, write_8, write_16, and write_array functions, making these
virtual to ensure that constructed types like arrays and sequences
correctly fragment their elements, and similar for structs, etc.

Old implementation attempted to implement fragmentation in the
TAO_OutputCDR inserters, but this does not work because array
and sequence types, including strings, were implemented at
the ACE_OutputCDR layer, which remained oblivious to fragmentation.
The old implementation also left TAO_OutputCDR with a dangerous
interface. The write_xxx function inherited from ACE_OutputCDR did
not fragment, only the inserters did.
The new implementation fragments in the low-level write_1, write_2,
write_4, write_8, write_16, and write_array functions, making these
virtual to ensure that constructed types like arrays and sequences
correctly fragment their elements, and similar for structs, etc.
@jwillemsen jwillemsen added the needs review Needs to be reviewed label Sep 12, 2020
@jwillemsen
Copy link
Member

Adding virtual will cost performance, a solution without virtual would be preferred

@j18ter
Copy link
Contributor Author

j18ter commented Sep 18, 2020

Adding virtual will cost performance, a solution without virtual would be preferred

Yes, understood, but avoiding virtual functions here is easier said than done because of the design of class ACE_OutputCDR and the derived TAO_OutputCDR class. The ACE base class implements the encoding of some of the complex types, most notably ordinary and wide strings, with optional encoders, and also the array types. String encoding requires encoding of basic types, i.e., the length fields, which requires correct fragmentation behaviour, and the arrays may need to get broken up into pieces, but fragmentation is implemented at the TAO layer. Given this current design based on inheritance, virtual functions are the obvious way to delegate fragmentation to the derived class when the the base class encodes basic types or arrays. Only a small number of functions need to be overridden.

Doing this without virtual functions will require more surgery to the design. One possibility would be to move the implementation of strings and associated encoders from the ACE layer to the TAO layer where the fragmentation support is available. A lot of implementation code would need to get moved, and I don't know if string encoding is required to be present at the ACE layer, i.e., when ACE_OutputCDR is used independent of TAO. Is it ever used that way?

If the ACE implementation for string encoding must remain in ACE, then it could be duplicated at the TAO layer with fragmentation added, but this duplication would be highly undesirable from a maintainability standpoint.

The only remaining possibility I see is to turn ACE_OutputCDR into a template class that takes a template parameter to customize the functions I had changed to virtual. This seems reasonable, but if we are not even allowed to use namespace std because we must support ancient compilers, how difficult will it be to avoid problems with template support for these ancient compilers?

Anyway, the point is that there are obvious bugs when fragmentation is enabled server-side. Virtual functions are an easy fix. Fixing it by using templates may be a more efficient alternative, but I don't have the resources to pursue this alternative fix, and to be honest, it is much fun to work on ACE/TAO source code given the constraints and coding style that are being imposed because of ancient compiler support.

@j18ter
Copy link
Contributor Author

j18ter commented Sep 18, 2020

Adding virtual will cost performance, a solution without virtual would be preferred

One more comment about the desire to avoid virtual functions for performance reasons: Although ACE_OutputCDR does not currently have virtual functions, the fragmentation_strategy_ member of TAO_OutputCDR involves virtual function calls already, and these are invoked at approximately the same rate as the proposed new virtual functions. Virtual function calls during CDR encoding are currently only avoided if one uses the ACE_OutputCDR class by itself, independent of TAO.

Actually, even ACE_OutputCDR already uses virtual function calls via its ACE_Char_Codeset_Translator pointer and the wide char variant.

@j18ter
Copy link
Contributor Author

j18ter commented Sep 18, 2020

By the way, I'm not sure why the VS2017WChar build failed. This happens because some PIDL files are apparently not having their stubs generated, but the pull request didn't make any changes in this locus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Needs to be reviewed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants