Iox #221 apply cxx list #224

bishibashiB · 2020-08-07T13:20:12Z

Within the attached source std::list usage was removed from roudi process list handling (replaced with cxx::list implementation).
Additionally the FixedPositionContainer of roudi port pool was picked to replace with cxx::list as well... see details in #221. (fixes #221)

This PR requires cxx::list to be approved (merged) #215 first.

elfenpiff · 2020-08-19T08:36:50Z

iceoryx_posh/include/iceoryx_posh/internal/roudi/iceoryx_port_pool.hpp

@@ -28,8 +28,8 @@ class IceOryxPortPool : public PortPool
  public:
    IceOryxPortPool(PortPoolData& portPool) noexcept;

-    cxx::vector<SenderPortType::MemberType_t*, MAX_PORT_NUMBER> senderPortDataList() noexcept override;
-    cxx::vector<ReceiverPortType::MemberType_t*, MAX_PORT_NUMBER> receiverPortDataList() noexcept override;
+    cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept override;


Uh this is dangerous. What if someone takes the list and modifies it arbitrarily? Is this what we want? If yes, then we can just make the whole list public, otherwise I would return here a copy.

see your comment below (restructuring) ... #257

Copy if efficiency is no concern, const& otherwise (depends who uses it, really). This does not protect against const_casts though but sends a strong message that it is read-only. If another API already requires modifying the list (i.e. we need a non const &) we need to take a further look.
Was a similar problem before with the vector, so I do not necessarily see it as in scope of this PR to fix this API design.

A discussion lead to the following proposal: return a container (probably still a vector if it is just read from) of pointers to the ports as is instead of a reference to the underlying container, which is now a list.
This has more overhead since this container needs to be constructed every time the function is called (this was also the case before in the content() function), but does not expose the internal list (but the ports, as before).

In the long run this can maybe be improved even more by passing the list reference as proposed, at the cost of losing encapsulation (the implications need to be checked carefully).

The underlying container should still be changed to a list to allow efficient insertion and removal.

elfenpiff · 2020-08-19T08:38:40Z

iceoryx_posh/include/iceoryx_posh/roudi/port_pool.hpp

-    cxx::vector<popo::InterfacePortData*, MAX_INTERFACE_NUMBER> interfacePortDataList() noexcept;
-    cxx::vector<popo::ApplicationPortData*, MAX_PROCESS_NUMBER> appliactionPortDataList() noexcept;
-    cxx::vector<runtime::RunnableData*, MAX_RUNNABLE_NUMBER> runnableDataList() noexcept;
+    virtual cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept = 0;


Again, it is dangerous to return the reference to internal members. Then everyone can modify, delete and add anything they like - even invalid data. If this is what we want make the list public otherwise we have to return a copy here.

see your comment below (restructuring via new issue) ... #257

Still depends on how it is used, sometimes breaking encapsulation for internal classes to acheive efficiency makes perfect sense. Regardless, the encapsulation was compromised before and ith is not necessarily in scope of this PR to fix this.

iceoryx_posh/source/roudi/port_manager.cpp

elfenpiff · 2020-08-19T08:52:56Z

iceoryx_posh/source/roudi/port_pool.cpp

    {
-        auto interfacePortData = m_portPoolDataBase->m_interfacePortMembers.insert(
+        auto interfacePortData = &m_portPoolDataBase->m_interfacePortMembers.emplace_front(
            iox::cxx::string<100>(iox::cxx::TruncateToCapacity, applicationName), interface);


No magic numbers. Why string<100>?
You could use the alias using ProcessName_t = cxx::string<100>; which can be found in iceoryx_posh_types.hpp

I know you did not write this particular line of code but the boyscout principle applies and you should always leave the code a little bit cleaner then you found it. 🦘

I understand it is valuable to remove any of those magic numbers. I fully agree!
For sure I will do a round-about approach to remove those- my proposal: in a follow-up issue / PR #257 (as the change would not be about this single line...)
Additionally there is something about using a type 'ProcessName_t' for an 'applicationName' which sparks my naming convention senses.
As I guess there is room for discussion on the number of name-types-in-use for process, application, runnable, service I leave this for discussions before creating any other precedents.

iceoryx_posh/source/roudi/port_pool.cpp

bishibashiB

Addressed comments from @elfenpiff
Ether committed or forwarded to #257

bishibashiB · 2020-08-24T13:46:28Z

iceoryx_posh/source/roudi/port_pool.cpp

@@ -25,27 +25,27 @@ PortPool::PortPool(PortPoolDataBase& portPoolDataBase) noexcept
 {
 }

-cxx::vector<popo::InterfacePortData*, MAX_INTERFACE_NUMBER> PortPool::interfacePortDataList() noexcept
+cxx::list<popo::InterfacePortData, MAX_INTERFACE_NUMBER>& PortPool::interfacePortDataList() noexcept


May I forward this point to a new issue? #257
I think we agree that copying a list is not in favour. So the interface between port_pool and roudi should be fostered.

iceoryx_posh/source/roudi/port_manager.cpp

bishibashiB · 2020-08-24T14:08:05Z

iceoryx_posh/source/roudi/port_manager.cpp

    {
-        iox::popo::ApplicationPort applicationPort(applicationPortData);
+        auto checkIter = iter;


see above...

iceoryx_posh/source/roudi/port_pool.cpp

bishibashiB · 2020-08-24T14:28:43Z

iceoryx_posh/include/iceoryx_posh/roudi/port_pool.hpp

-    cxx::vector<popo::InterfacePortData*, MAX_INTERFACE_NUMBER> interfacePortDataList() noexcept;
-    cxx::vector<popo::ApplicationPortData*, MAX_PROCESS_NUMBER> appliactionPortDataList() noexcept;
-    cxx::vector<runtime::RunnableData*, MAX_RUNNABLE_NUMBER> runnableDataList() noexcept;
+    virtual cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept = 0;


see your comment below (restructuring via new issue) ... #257

bishibashiB · 2020-08-24T14:30:01Z

iceoryx_posh/include/iceoryx_posh/internal/roudi/iceoryx_port_pool.hpp

@@ -28,8 +28,8 @@ class IceOryxPortPool : public PortPool
  public:
    IceOryxPortPool(PortPoolData& portPool) noexcept;

-    cxx::vector<SenderPortType::MemberType_t*, MAX_PORT_NUMBER> senderPortDataList() noexcept override;
-    cxx::vector<ReceiverPortType::MemberType_t*, MAX_PORT_NUMBER> receiverPortDataList() noexcept override;
+    cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept override;


see your comment below (restructuring) ... #257

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_handler.inl

MatthiasKillat · 2020-09-14T14:59:36Z

iceoryx_posh/include/iceoryx_posh/internal/roudi/iceoryx_port_pool.hpp

@@ -28,8 +28,8 @@ class IceOryxPortPool : public PortPool
  public:
    IceOryxPortPool(PortPoolData& portPool) noexcept;

-    cxx::vector<SenderPortType::MemberType_t*, MAX_PORT_NUMBER> senderPortDataList() noexcept override;
-    cxx::vector<ReceiverPortType::MemberType_t*, MAX_PORT_NUMBER> receiverPortDataList() noexcept override;
+    cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept override;


Copy if efficiency is no concern, const& otherwise (depends who uses it, really). This does not protect against const_casts though but sends a strong message that it is read-only. If another API already requires modifying the list (i.e. we need a non const &) we need to take a further look.
Was a similar problem before with the vector, so I do not necessarily see it as in scope of this PR to fix this API design.

MatthiasKillat · 2020-09-14T15:03:54Z

iceoryx_posh/include/iceoryx_posh/roudi/port_pool.hpp

-    cxx::vector<popo::InterfacePortData*, MAX_INTERFACE_NUMBER> interfacePortDataList() noexcept;
-    cxx::vector<popo::ApplicationPortData*, MAX_PROCESS_NUMBER> appliactionPortDataList() noexcept;
-    cxx::vector<runtime::RunnableData*, MAX_RUNNABLE_NUMBER> runnableDataList() noexcept;
+    virtual cxx::list<SenderPortType::MemberType_t, MAX_PORT_NUMBER>& senderPortDataList() noexcept = 0;


Still depends on how it is used, sometimes breaking encapsulation for internal classes to acheive efficiency makes perfect sense. Regardless, the encapsulation was compromised before and ith is not necessarily in scope of this PR to fix this.

MatthiasKillat · 2020-09-14T15:09:22Z

iceoryx_posh/include/iceoryx_posh/roudi/port_pool.hpp

@@ -75,8 +75,8 @@ class PortPool
                    const iox::cxx::CString100& runnable,
                    const uint64_t runnableDeviceIdentifier) noexcept;

-    virtual void removeSenderPort(SenderPortType::MemberType_t* const portData) noexcept = 0;
-    virtual void removeReceiverPort(ReceiverPortType::MemberType_t* const portData) noexcept = 0;
+    virtual void removeSenderPort(const SenderPortType::MemberType_t& portData) noexcept = 0;


Apart from pointer to ref the semantics changed here as well, was a const pointer but mutable data before (which is not that useful in function arguments, same as passing a const int), now the data is immutable. The function seems not to modify it (i.e. it compiles), so it is Ok.
Just an observation that it is stricter now.

MatthiasKillat · 2020-09-14T15:21:52Z

iceoryx_posh/source/popo/sender_port.cpp

@@ -266,9 +266,9 @@ void SenderPort::deliverChunkToAllReceiver(const mepoo::SharedChunk f_chunk)

    auto& receiverList = getMembers()->m_receiverHandler.appContext().getReceiverList();

-    for (int64_t i = static_cast<int64_t>(receiverList.size()) - 1; i >= 0; --i)
+    for (auto& receiver : receiverList)


Well, I guess the order was not important anyway (was reverse before, for whatever reason). It should only be important that it is delivered to every receiver, not in which order.

MatthiasKillat · 2020-09-15T11:47:22Z

iceoryx_posh/source/roudi/iceoryx_port_pool.cpp

@@ -26,15 +26,15 @@ IceOryxPortPool::IceOryxPortPool(PortPoolData& portPoolData) noexcept
 }

 /// @deprecated #25
-cxx::vector<SenderPortType::MemberType_t*, MAX_PUBLISHERS> IceOryxPortPool::senderPortDataList() noexcept
+cxx::list<SenderPortType::MemberType_t, MAX_PUBLISHERS>& IceOryxPortPool::senderPortDataList() noexcept


Returning the container by reference now is intentional? (For efficiency?) Before we returned a container filled with pointers, now we return a reference to the container. The internal elements are exposed either way. This way the container is also exposed for modification (more misuse potential).

MatthiasKillat · 2020-09-15T12:06:41Z

iceoryx_posh/source/roudi/iceoryx_port_pool.cpp

 {
-    m_portPoolData->m_senderPortMembers.erase(portData);


Ok, worked before because the iterators where pointers (for cxx::vector). Now to ensure deletion we need to provide a reference to an object actually in the container (equivalent to a pointer in this regard). Should be fine, but worth thinking about that it is still correct and equivalent to before. We cannot delete by id or name or anything else. The address of the object to be deleted is essentially the id.

We cannot delete by id or name or anything else. The address of the object to be deleted is essentially the id.

Q: Could there appear a race where the pointer-is-ID approach becomes corrupted due to different address ranges in processes... ?

elBoberido · 2020-09-22T10:07:11Z

iceoryx_posh/include/iceoryx_posh/internal/popo/receiver_handler.hpp

@@ -53,7 +53,7 @@ class ReceiverHandler : public LockingPolicy
    using this_type = ReceiverHandler_t;

  public:
-    using ReceiverVector_t = cxx::vector<relative_ptr<ReceiverPortType::MemberType_t>, MaxReceivers>;
+    using ReceiverList_t = cxx::list<relative_ptr<ReceiverPortType::MemberType_t>, MaxReceivers>;


I'm not sure if this is really a good replacement. While theoretically a list would be a good idea, this list is rarely changed, but often read and therefore it might be faster if it remains a vector, even if it means that removing items will be quite slow

You may be right here. Cache Locality is much worse with the list and this is probably the reason for a large performance loss while iterating over its elements.

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

elBoberido

I just wanted to propose to split this up into multiple PR since I feared you would beat me for the longest open PR

mossmaurice requested review from elBoberido, mossmaurice, budrus and elfenpiff August 14, 2020 08:10

mossmaurice added the enhancement New feature label Aug 14, 2020

bishibashiB force-pushed the feature/iox-#221_apply_cxx_list branch 2 times, most recently from 2273d40 to b7f9475 Compare August 14, 2020 13:34

elfenpiff requested changes Aug 19, 2020

View reviewed changes

bishibashiB commented Aug 24, 2020

View reviewed changes

bishibashiB requested a review from elfenpiff September 1, 2020 07:14

bishibashiB force-pushed the feature/iox-#221_apply_cxx_list branch from 2161556 to bf03943 Compare September 15, 2020 06:29

MatthiasKillat reviewed Sep 15, 2020

View reviewed changes

bishibashiB changed the title ~~Feature/iox #221 apply cxx list~~ Iox #221 apply cxx list Sep 16, 2020

elBoberido reviewed Sep 22, 2020

View reviewed changes

mossmaurice added this to the v1.x milestone Nov 4, 2020

iox-eclipse-iceoryx#221 replace occurenece of std::list

ff08d01

Signed-off-by: Eickhoff Bernhard (CC-AD/ESW1) <bernhard.eickhoff@de.bosch.com>

bishibashiB force-pushed the feature/iox-#221_apply_cxx_list branch from d7c5b59 to ff08d01 Compare November 24, 2020 08:17

elBoberido approved these changes Nov 24, 2020

View reviewed changes

elfenpiff approved these changes Nov 25, 2020

View reviewed changes

elfenpiff merged commit 65445be into eclipse-iceoryx:master Nov 25, 2020

bishibashiB deleted the feature/iox-#221_apply_cxx_list branch November 25, 2020 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox #221 apply cxx list #224

Iox #221 apply cxx list #224

bishibashiB commented Aug 7, 2020 •

edited

Loading

elfenpiff Aug 19, 2020

bishibashiB Aug 24, 2020

MatthiasKillat Sep 14, 2020

MatthiasKillat Sep 18, 2020 •

edited

Loading

elfenpiff Aug 19, 2020

bishibashiB Aug 24, 2020

MatthiasKillat Sep 14, 2020

elfenpiff Aug 19, 2020

bishibashiB Aug 24, 2020

bishibashiB left a comment •

edited

Loading

bishibashiB Aug 24, 2020

bishibashiB Aug 24, 2020

bishibashiB Aug 24, 2020

bishibashiB Aug 24, 2020

MatthiasKillat Sep 14, 2020

MatthiasKillat Sep 14, 2020

MatthiasKillat Sep 14, 2020

MatthiasKillat Sep 14, 2020

MatthiasKillat Sep 15, 2020

MatthiasKillat Sep 15, 2020

bishibashiB Sep 16, 2020

elBoberido Sep 22, 2020

MatthiasKillat Sep 24, 2020

elBoberido left a comment

Iox #221 apply cxx list #224

Iox #221 apply cxx list #224

Conversation

bishibashiB commented Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bishibashiB left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

bishibashiB commented Aug 7, 2020 •

edited

Loading

MatthiasKillat Sep 18, 2020 •

edited

Loading

bishibashiB left a comment •

edited

Loading