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

Variable length message fields allocation free [7550] #1003

Merged
merged 51 commits into from
Feb 10, 2020
Merged

Variable length message fields allocation free [7550] #1003

merged 51 commits into from
Feb 10, 2020

Conversation

IkerLuengo
Copy link
Contributor

Reworked to keep API compatibility

@IkerLuengo
Ref. 5750 XML schema for variable size parameters' max size
2a6b2a6
@IkerLuengo
Ref. 5750 make data types deserialize themselves
df1ebae
@IkerLuengo
Ref. 5750 Make ParticipantProxyData deserialize its messages
e7aa067
@IkerLuengo
Ref. 5750 Make ReaderProxyData deserialize its messages
dd79e00
@IkerLuengo
Ref. 5750 Make WriterProxyData deserialize its messages
ef268f2
@IkerLuengo
Ref. 5750 avoid allocations on deserialize
2da140e
@IkerLuengo
Ref. 5750 Make UserData aware of it own size
2b37372
@IkerLuengo
Ref. 5750 max size-aware ParameterPropertyList_t
5655564
@IkerLuengo
Ref. 5750 group data limits in a new struct
88363df
@IkerLuengo
Ref. 5750 initialize data limits on WriterProxyData and ReaderProxyData
ce38603
@IkerLuengo
Ref. 5750 more initialize data limits on WriterProxyData and ReaderPr…
3d75fb1 
…oxyData
@IkerLuengo
size limit on PartitionQosPolicy
6725e35
@IkerLuengo
Ref. 5750 Improve properties' modifications
a7031bd 
Improve error handling while modifying properties
@IkerLuengo
Ref. 5750 testing dynamic data size limits
0ae0848
@IkerLuengo
Ref. 5750 remove obsolete code
0f7e175
@IkerLuengo
Ref. 5750 Remove unused ParameterList::readParameterListfromCDRMsg
12e6439
@IkerLuengo
Ref. 5750 correct compilation with security option
76ffe3d
@IkerLuengo
Ref. 5750 correct compilations on mocks
c18633d
@IkerLuengo
Correct compilation warnings
0d70e0b
@IkerLuengo
Correct security manager message reading routine
abb81b2
@IkerLuengo
remove compilation warning
c5dc730
@IkerLuengo
Correct valgrind errors
0dbaa8f
@IkerLuengo
Remove compilation warnings on Windows
6c738a6
@IkerLuengo
Reduce code duplication in CDRMessage::add_string
ed797ae 
And applying some naming rules
@IkerLuengo
Correct logic error on SerializedPayload::operator==
e4b18c3
@IkerLuengo
Uncrustify CDRMessage
90c4d76
@IkerLuengo
Remove unused method on ParameterList
4feb3b8
@IkerLuengo
correct string size alignment
3e47f99
@IkerLuengo
Ref. 5750 ParameterType checks size when reading from message
e29cbbf
@IkerLuengo
Ref. 5750 Move comon code of reading messages to ParameterLists
15359a7
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

MiguelCompany
MiguelCompany requested changes Feb 10, 2020
View reviewed changes
Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On both QosPolicies.h (public and mock), on the simple clear methods, I think it is better to have a constexpr and a copy than a stack local and a swap.

src/cpp/rtps/builtin/data/ParticipantProxyData.cpp Outdated Show resolved Hide resolved
@IkerLuengo
Copy link
Contributor Author

On both QosPolicies.h (public and mock), on the simple clear methods, I think it is better to have a constexpr and a copy than a stack local and a swap.

We cannot have a constexpr because the destructor of the classes is not trivial (as the base classes have virtual destructors)

@IkerLuengo
Copy link
Contributor Author

We were sending UserData and Partitions always (even if empty) because 'hasChanged' was set to true on clear, instead of resetting it to its default value.

TypeConsistencyEnforcement reading would fail if size > 7.

MiguelCompany
MiguelCompany previously approved these changes Feb 10, 2020
View reviewed changes
@IkerLuengo IkerLuengo force-pushed the feature/allocation_qos_reworked/1.9.x branch from 0be71d4 to 8e87789 Compare February 10, 2020 11:24
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany MiguelCompany added this to the v1.9.5 milestone Feb 10, 2020
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany MiguelCompany merged commit c313101 into 1.9.x Feb 10, 2020
@MiguelCompany MiguelCompany deleted the feature/allocation_qos_reworked/1.9.x branch February 10, 2020 14:49
@IkerLuengo IkerLuengo mentioned this pull request Feb 12, 2020
Merged
IkerLuengo added a commit that referenced this pull request Feb 13, 2020
@IkerLuengo
merge latest commits from #1003
7dfd658 
Includes the following commits

* 752b865 reset 'hasChanged' to default value on clear

* de0f4e7 correct reading of TypeConsistencyEnforcement

Also includes some corrections to leave objects on default state when
QosPolicies::clear()
This was referenced Feb 13, 2020
Closed
Closed
@dirk-thomas
Copy link
Contributor

This change broke cross vendor compatibility with Connext: see #1014.

@IkerLuengo IkerLuengo mentioned this pull request Feb 14, 2020
Merged
IkerLuengo added a commit that referenced this pull request Feb 20, 2020
@IkerLuengo
merge latest commits from #1003
ff3ca46 
Includes the following commits

* 752b865 reset 'hasChanged' to default value on clear

* de0f4e7 correct reading of TypeConsistencyEnforcement

Also includes some corrections to leave objects on default state when
QosPolicies::clear()
IkerLuengo added a commit that referenced this pull request Feb 28, 2020
@IkerLuengo
merge latest commits from #1003
f3671a7 
Includes the following commits

* 752b865 reset 'hasChanged' to default value on clear

* de0f4e7 correct reading of TypeConsistencyEnforcement

Also includes some corrections to leave objects on default state when
QosPolicies::clear()
MiguelCompany pushed a commit that referenced this pull request Mar 3, 2020
@IkerLuengo
Variable length message fields allocation free (#1004)
a999b3a 
* Refs 7529 Port of PR 965

2fb801734 Ref. 5750 clear QoS instead of reconstruct

39cb3e5 apply review suggestions

50dbc8c Refs #7305. Revert test modifications to old API

ccec5bd Refs.7305 Restore API on QosPolicies

4c2b426 Refs #7305. Style on PKIDH.cpp

ece3c00 Refs #7305. Using ParameterList::read_guid_from_cdr_msg on PKIDH.

ad52820 Refs #7305. Style on ParameterList.cpp

582be58 Refs #7305. Added ParameterList::read_guid_from_cdr_msg.

e1ccfea Refs #7305. Styling on WriterProxyData.

b7fa7a8 Refs #7305. Styling on ReaderProxyData.

4471db2 Refs #7305. Styling on ParticipantProxyData.

3bc368a Refs #7305. Avoid duplicate code on ParameterList.cpp

419108b Refs #7305. Uncrustify QosPolicies.

d2ba714 Refs #7305. Uncrustify ParameterTypes

d9260b6 Ref. 5750 unused variable warning removal

9200aa3 Ref. 5750 ProxyDatas don't need data limits anymore

f2f22d4 Ref. 5750 Removing warnings

15359a7 Ref. 5750 Move comon code of reading messages to ParameterLists

e29cbbf Ref. 5750 ParameterType checks size when reading from message

3e47f99 correct string size alignment

4feb3b8 Remove unused method on ParameterList

90c4d76 Uncrustify CDRMessage

e4b18c3 Correct logic error on SerializedPayload::operator==

ed797ae Reduce code duplication in CDRMessage::add_string

6c738a6 Remove compilation warnings on Windows

0dbaa8f Correct valgrind errors

c5dc730 remove compilation warning

abb81b2 Correct security manager message reading routine

0d70e0b Correct compilation warnings

c18633d Ref. 5750 correct compilations on mocks

76ffe3d Ref. 5750 correct compilation with security option

12e6439 Ref. 5750 Remove unused ParameterList::readParameterListfromCDRMsg

0f7e175 Ref. 5750 remove obsolete code

0ae0848 Ref. 5750 testing dynamic data size limits

a7031bd Ref. 5750 Improve properties' modifications

6725e35 size limit on PartitionQosPolicy

3d75fb1 Ref. 5750 more initialize data limits on WriterProxyData and ReaderProxyData

ce38603 Ref. 5750 initialize data limits on WriterProxyData and ReaderProxyData

88363df Ref. 5750 group data limits in a new struct

5655564 Ref. 5750 max size-aware ParameterPropertyList_t

2b37372 Ref. 5750 Make UserData aware of it own size

2da140e Ref. 5750 avoid allocations on deserialize

ef268f2 Ref. 5750 Make WriterProxyData deserialize its messages

dd79e00 Ref. 5750 Make ReaderProxyData deserialize its messages

e7aa067 Ref. 5750 Make ParticipantProxyData deserialize its messages

df1ebae Ref. 5750 make data types deserialize themselves

2a6b2a6 Ref. 5750 XML schema for variable size parameters' max size

* uncrustify

* merge latest commits from #1003

Includes the following commits

* 752b865 reset 'hasChanged' to default value on clear

* de0f4e7 correct reading of TypeConsistencyEnforcement

Also includes some corrections to leave objects on default state when
QosPolicies::clear()

* Refs 7529 Correct missing override tag

* Store endpoint GUID from message

This is a port of the bugfix #1015

* Initialize dynamic types on new ProxyData ctor

* Refs 7529. Apply review suggestions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiguelCompany MiguelCompany MiguelCompany approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@IkerLuengo @richiware @dirk-thomas @MiguelCompany