Fixed size buffers manager #1807

valegagge · 2018-07-19T08:04:25Z

In this PR I developed the FixedSizeBufferManager, a manager of fixed size buffers for multi-threads environment.
The buffers are allocated when the manager is created and the user can get a buffer, uses and releases it when he/she doesn't need anymore. If all buffers are busy and a new buffer is required, than the manager creates new one.

I used FixedSizeBufferManager in the "implementation" layer of Control Board interfaces in order to remove all dynamic allocations.. Please see #1684.

Nicogene

I didn't check the code in its functionality, the amount of peaky comments is due to the fact that introducing new classes in YARP_OS is very delicate, sorry 😅
Speaking about, I'm not sure that these classes( as the one of this PR #1516 ) should stay in YARP_OS, but another proper library should be created(cc @drdanz) .

The last 2 comments:

Missing some lines in the changelog.
Missing tests

The last point is very important, each new class introduced should have a test, otherwise maintain it become very difficult.

Probably this PR should be merged in yarp 3.2.0, since there is the feature freeze until 3.1.0 is released(cc @drdanz).

Nicogene · 2018-07-19T09:00:01Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager-inl.h

+
+
+template <typename T>
+yarp::os::Buffer<T>::Buffer():key(0), dataPtr(nullptr), numOfElements(0){;}


Nicogene · 2018-07-19T09:00:31Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager-inl.h

+yarp::os::Buffer<T>::Buffer():key(0), dataPtr(nullptr), numOfElements(0){;}
+
+template <typename T>
+yarp::os::Buffer<T>::~Buffer(){;}


Nicogene · 2018-07-19T09:05:15Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager-inl.h

+    if(index<numOfElements)
+        return dataPtr[index];
+    else
+        throw std::out_of_range("yarp::os::Buffer::getValue(index): index is out f range");


Are we sure to introduce exceptions in yarp?
They are a good thing if they are handled through try{} catch{} statement, bad otherwise.
In other yarp containers if the index is out of range usually a dummy object is returned ( return T();)

The behaviour of buffer is quite equal to std::vector regarding excpetions: std::vector lauches an std::out_of_range exception when a std::vector::at(index) function is called with a wrong index; in the same way, yarp::os::Buffer launches the same exception when set/get functions are invoked with a wrong index.
Both std::vector and yarp::os::Buffer has operator [] that lets user to access an element without any check of index.

Moreover in yarp we use the std::vector and if no try-catch statements is used is a developer's choise.
For example I dind't use the try-catch in implementation layer, because when I access to a buffer's element I'm sure that the index is minor of its size.

Nicogene · 2018-07-19T09:08:27Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+     * If @index is not minor of size of buffer than a std::out_of_range is thrown.
+     *
+     */
+    void setValue(uint32_t index, T value) throw (std::out_of_range);


See the other comment about exceptions

Nicogene · 2018-07-19T09:09:19Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+
+#include "FixedSizeBuffersManager-inl.h"
+
+#endif // YARP_OS_FIXEDSIZEBUFFERSMANAGER_H


Missing endline at the end of the file

Nicogene · 2018-07-19T09:15:28Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+
+// Defined in this file:
+namespace yarp { namespace os { template <typename T> class FixedSizeBuffersManager; }}
+namespace yarp { namespace os { template <typename T> class Buffer; }}


You can remove these 2 forward declarations, you don't need them

sorry, you are wrong.
We need them because `FixedSizeBufferManager' is a friend class of Buffer.

You are right ! Yet another reason why I don't like friend classes 😬

Nicogene · 2018-07-19T09:20:53Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager-inl.h

+    m_mutex.lock();
+    //get fisrt  free buffer
+    Buffer<T> buffer;
+    uint32_t i;


Missing initialization of i

Nicogene · 2018-07-19T09:25:50Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+
+private:
+    uint32_t key;
+    T* dataPtr;


I don't know how is feasible but for new classes I discourage the usage of C pointers, I'd prefer std::unique_ptr.
unique_ptr has overhead only in constructor and destructor if they are not trivial, but it is not the case if T is a literal type.

I think std::unique_ptr is a good feature that let developer to forget delete statements, but in this case I prefer continue to use pointer C style, because this is a Manager of buffer and its abojective is alloc and dealloc memory in the right moment.

Moreover, I already tested the buffer and it works.
So I don't see the need to change something to seems ok without add real improvements.

The std::unique_ptr can be use in user-code where it is necessary to allocate and to deallocate memory most frequently, in order to avoid the usual bug about delete (forgot a delete or delete something not exists anymore)

I think the Buffer does not own the memory pointed by dataPtr (that is actually owned by FixedSizeBuffersManager, and "shared" with the Buffer object), so I do not think that unique_ptr can be used here.

Nicogene · 2018-07-19T09:27:00Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+*/
+
+template <typename T>
+class Buffer


Missing YARP_OS_api

Nicogene · 2018-07-19T09:27:12Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager.h

+ *
+ */
+template <typename T>
+class FixedSizeBuffersManager


Missing YARP_OS_api

Nicogene

Discussed f2f with @valegagge, long story short:
This class will be integrated in yarp 3.2.0, probably in a different library respect YARP_OS, for this reason changelog and tests are not already provided.
All the comment about the implementation will be ignored for now, and rekindled once we will find a place for these classes 😅

But Travis and Appveyor are failing, we should fix it 🤔

valegagge · 2018-07-20T08:33:57Z

Since the FixedSizeBuffer and Buffer classes have been developed for general porpouse, I think their should be added in the basic part of yarp, where there are all classes general porpouse. I know that @drdanz and you are working to separate general porpouse code in a approppriate namespace. So, please feel free to move these classes in the new namespace.

If you need a start point to develop test you can use this files:
https://github.com/valegagge/my_appls/blob/master/buffersHelperTest_RFModule/src/buffHelpTest.cpp
https://github.com/valegagge/my_appls/blob/master/buffersHelperTest_RFModule/src/buffHelpTest.h
They are not the last version, but I lost it. sorry!

randaz81 · 2018-07-20T09:03:03Z

There are some typos in the comments, so I recommended to turn on the spell checker in the IDE.
Probably it is a good idea to complete tests and add them to this PR before merging.

randaz81 · 2018-07-20T09:14:53Z

I would rename class Buffer into something else, e.g. ManagedBuffer.
It is better to avoid names that are too general, they may generate confusion, especially in yarp that already employs classes with similar names.

traversaro · 2018-07-20T09:19:00Z

General comment: the yarp::os::Buffer, except for the key attribute and the friendship discussed in https://github.com/robotology/yarp/pull/1807/files#r203654714 , is quite similar to the upcoming C++20 std::span class, for which we discussed introducing a YARP-compatible equivalent in #1750 (comment) and #1582 (comment) (we already did something similar in iDynTree that can be directly imported, see robotology/idyntree#434).

I am not really sure if it is fair to block @valegagge work on this, but perhaps it could make sense to refactor the yarp::os::Buffer in something std::span API-compatible and just have a small class/struct that contains a yarp::Span (regardless of how it is actually called) and a key for the use of the FixedSizeBufferManager. @drdanz @Nicogene

randaz81 · 2018-07-20T09:34:27Z

src/libYARP_dev/src/ImplementControlMode.cpp

-    int *modes_tmp=new int [nj];
-    for(int idx=0; idx<nj; idx++)
+    Buffer<int> buffValues  = buffManager->getBuffer();
+    for(int idx=0; idx<castToMapper(helper)->axes(); idx++)


I would simplify the code using nj instead castToMapper(helper)->axes().
Same in all other interfaces.
Additionally, we are in the process of replacing all these int to size_t.

@randaz81 for semplify do you mean "more performing" or "more readable"?
I think that in both case with don't get an andvantage
Anyway feel free to sobstitute nj with castToMapper(helper)->axes() and int with size_t.

just readable.. I can't find any good reason to ask to some else an info that I already know...

valegagge · 2018-07-20T11:27:58Z

About the name of class Buffer, you can choose wich you prefer, then we need to find/replace only in all implement control board interfaces.
For me is ok ManagedBuffer or we can change the word buffer in array. Other proposals?

Instead about std::span, if someone would like to change the implementation can do (since code is open) but before I would like to merge this PR.

Nicogene · 2018-07-20T15:50:25Z

I like very much the idea of @traversaro to uniform the class to the std::span in order to, one day, replace it when we will support c++20.

He is right about not stopping the work of @valegagge , my advice is to fix the typos and minor issues of this PR, make happy travis and appveyor(I didn't check what is the problem) and make these classes impl, so invisible to the user for now, but used by us internally in the ControlBoardImpl.

This will give us more freedom, in the future changing its name, api etc without any problem of backward compatiblity

Nicogene · 2018-07-20T15:58:19Z

Tip for fixing typos:

$ pip install codespell
$ cd $YARP_ROOT/src/libYARP_OS/include/yarp/os
$ codespell -w ./FixedSizeBuffersManager.h  ./FixedSizeBuffersManager-inl.h

drdanz

To be honest I'm not confortable with adding new classes to yarp::os, I'm moving it to yarp/dev/impl...

drdanz · 2018-08-02T15:54:18Z

src/libYARP_OS/include/yarp/os/FixedSizeBuffersManager-inl.h

+uint32_t yarp::os::Buffer<T>::getSize() {return numOfElements;}
+
+template <typename T>
+T  yarp::os::Buffer<T>::getValue(uint32_t index) throw (std::out_of_range)


throw (std::out_of_range) is deprecated

drdanz · 2018-08-02T15:55:32Z

src/libYARP_dev/src/ImplementPositionDirect.cpp

-{
-}
+    doubleBuffManager(nullptr),
+    intBuffManager(nullptr)


This causes a -Wreorder warning

drdanz · 2018-08-02T15:56:02Z

src/libYARP_dev/src/ImplementPositionDirect.cpp

+    Buffer<int> buffJoints = intBuffManager->getBuffer();
+    Buffer<double> buffValues = doubleBuffManager->getBuffer();
+
+    if(n_joint > intBuffManager->getBufferSize())


This causes a -Wsign-compare warning

Nicogene · 2018-08-30T16:28:56Z

The failures are in ControlBoardRemapperTest both in appveyor and travis.

valegagge · 2018-09-25T08:12:22Z

Hi @drdanz,
if I understand the log of appVeyor in the right way, the error is due to controlBoardremapperTest.
I run the same test on devel branch and it fails anyway.

How can we move on?

…izeBuffersManager

…ntrol::checkMotionDone with FixedSizeBuffersManager

…Control

Nicogene · 2018-10-03T12:14:19Z

Since now they have move to the impl part of YARP_dev, no update to the changelog is required.
Am I right @drdanz ?

Nicogene

LGTM, if it has been successfully tested on the robots we can merge it 👍

drdanz · 2018-10-03T12:41:35Z

Since now they have move to the impl part of YARP_dev, no update to the changelog is required.

Confirming this. Perhaps we should rebase this over master if it became just bugfix, but I'm not 100% sure it is worth, @valegagge what do you think?
Anyway, let's merge this if it finally builds, we can cherry-pick the branch later

drdanz · 2018-10-03T12:43:16Z

Actually there are several changes in the "protected" part of the public interfaces, therefore it should not be in master, please forget about my previous comment.

drdanz · 2018-10-03T16:49:38Z

Merged, thanks.

valegagge requested a review from drdanz July 19, 2018 08:04

valegagge requested review from barbalberto and randaz81 as code owners July 19, 2018 08:04

Nicogene requested changes Jul 19, 2018

View reviewed changes

Nicogene reviewed Jul 19, 2018

View reviewed changes

Nicogene changed the title ~~Fixed size buffers manager~~ [WIP] Fixed size buffers manager Jul 19, 2018

Nicogene assigned drdanz, valegagge and Nicogene Jul 19, 2018

randaz81 reviewed Jul 20, 2018

View reviewed changes

drdanz force-pushed the fixedSizeBuffersManager branch from 8e1e090 to 4f70a12 Compare August 2, 2018 16:24

drdanz suggested changes Aug 2, 2018

View reviewed changes

drdanz force-pushed the fixedSizeBuffersManager branch from 4f70a12 to 113976f Compare August 2, 2018 16:27

drdanz added Target: YARP v3.2.0 and removed Target: YARP v3.2.0 labels Sep 14, 2018

valegagge force-pushed the fixedSizeBuffersManager branch from 113976f to 304c0b8 Compare September 24, 2018 11:09

valegagge and others added 10 commits October 3, 2018 13:51

Implemented FixedSizeBuffersManager

96bcde9

Use FixedSizeBuffersManager in impl-layer of control board interfaces

17e8dc5

Used FixedSizeBuffersManager in impl-layer of control interfaces

a35017a

Some fix in impl-layer of control board interface about use of FixedS…

53d2ef5

…izeBuffersManager

Optimized of used of FixedSizeBuffersManager

858b426

Updated ImplementControlMode::setControlModes and ImplementPositionCo…

cbcfd6e

…ntrol::checkMotionDone with FixedSizeBuffersManager

Cleanup and move FixedSizeBuffersManager in dev/impl

f2f4701

fix bug: mssing boolBufferManager initialization in ImplementPosition…

3bb8b08

…Control

fixedSizeBuffersManager: fix typo

8d75ad3

Fix bug in ControlBoardHelper::checkAxesIds

d6b3eb4

valegagge force-pushed the fixedSizeBuffersManager branch from a812ffa to d6b3eb4 Compare October 3, 2018 11:54

Nicogene approved these changes Oct 3, 2018

View reviewed changes

drdanz approved these changes Oct 3, 2018

View reviewed changes

valegagge changed the title ~~[WIP] Fixed size buffers manager~~ Fixed size buffers manager Oct 3, 2018

drdanz merged commit 1248a5b into robotology:devel Oct 3, 2018

drdanz added the Resolution: Merged label Oct 3, 2018



		template <typename T>
		yarp::os::Buffer<T>::Buffer():key(0), dataPtr(nullptr), numOfElements(0){;}


		#include "FixedSizeBuffersManager-inl.h"

		#endif // YARP_OS_FIXEDSIZEBUFFERSMANAGER_H

Fixed size buffers manager #1807

Fixed size buffers manager #1807

Conversation

valegagge commented Jul 19, 2018

Nicogene 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nicogene left a comment

Choose a reason for hiding this comment

valegagge commented Jul 20, 2018

randaz81 commented Jul 20, 2018

randaz81 commented Jul 20, 2018

traversaro commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valegagge commented Jul 20, 2018

Nicogene commented Jul 20, 2018 • edited Loading

Nicogene commented Jul 20, 2018 • edited Loading

drdanz left a 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

Nicogene commented Aug 30, 2018

valegagge commented Sep 25, 2018

Nicogene commented Oct 3, 2018

Nicogene left a comment

Choose a reason for hiding this comment

drdanz commented Oct 3, 2018

drdanz commented Oct 3, 2018 • edited Loading

drdanz commented Oct 3, 2018

Nicogene left a comment •

edited

Loading

traversaro commented Jul 20, 2018 •

edited

Loading

Nicogene commented Jul 20, 2018 •

edited

Loading

Nicogene commented Jul 20, 2018 •

edited

Loading

drdanz commented Oct 3, 2018 •

edited

Loading