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

Improve net/transport code #156

Merged
merged 17 commits into from Sep 4, 2015

Conversation

Projects
None yet
2 participants
@bassosimone
Member

bassosimone commented Aug 31, 2015

This pull request returns to the net/transport code and prepares it for the upcoming release. It either simplifies code where excessive complexity exists, or it implements missing features.

More details in the individual commit messages. More can be done to improve this area of measurement-kit. But given the pending release and since I did not want to fuzz the patch too much, this pull request only contains what I consider to be the most urgent fixes.

@bassosimone bassosimone added this to the measurement-kit 0.1.0 milestone Aug 31, 2015

@@ -328,7 +327,7 @@ class Stream {
*/
void on_connect(std::function<void(void)>&& fn) {
connect_handler = fn;
connection->on_connect([this]() {
connection.on_connect([this]() {

This comment has been minimized.

@hellais

hellais Sep 2, 2015

Contributor

Just to understand the rational. You think the memory usage reduction is not worth complicating the code? If so, what is the benefit of not using the shared pointer?

@hellais

hellais Sep 2, 2015

Contributor

Just to understand the rational. You think the memory usage reduction is not worth complicating the code? If so, what is the benefit of not using the shared pointer?

This comment has been minimized.

@bassosimone

bassosimone Sep 2, 2015

Member

The benefit of not using a shared pointer is simpler code (hence simpler review and most importantly simpler debugging). On the contrary the benefit of using a shared pointer is that the pointee is alive as long as the shared pointer is alive. At a certain point this looked promising because it could allow one to emulate the semantic of languages like JavaScript. In particular I explored the possibility of using lambda closures to keep objects alive.

However, after trying this strategy out, I become convinced it was not so good.The code was becoming more complex to read. It was not very reliable to store objects inside the lambda closure (strange implementation issues, leaks because there is no cycle-aware garbage collector, etc.). As an example, see the issues reported in #134 and #111.

So I decided the wisest thing to do was to remove complexity (and probably it would have been better to apply KISS since the beginning). This payed off because I finally understood, for example, the reason of #134 (the fix for which is in a branch that will soon become a pull request).

@bassosimone

bassosimone Sep 2, 2015

Member

The benefit of not using a shared pointer is simpler code (hence simpler review and most importantly simpler debugging). On the contrary the benefit of using a shared pointer is that the pointee is alive as long as the shared pointer is alive. At a certain point this looked promising because it could allow one to emulate the semantic of languages like JavaScript. In particular I explored the possibility of using lambda closures to keep objects alive.

However, after trying this strategy out, I become convinced it was not so good.The code was becoming more complex to read. It was not very reliable to store objects inside the lambda closure (strange implementation issues, leaks because there is no cycle-aware garbage collector, etc.). As an example, see the issues reported in #134 and #111.

So I decided the wisest thing to do was to remove complexity (and probably it would have been better to apply KISS since the beginning). This payed off because I finally understood, for example, the reason of #134 (the fix for which is in a branch that will soon become a pull request).

This comment has been minimized.

@bassosimone

bassosimone Sep 2, 2015

Member

Reading more carefully the header file, since connection is a transport, nothing really changed in this case since before. That is, connection used to be SharedPointer<Transport>. Now Transport (the abstract thing) is TransportInterface and Transport contains a SharedPointer<TransportInterface>. So, at the end of the day, here what really changed is that using transport is easier since the template is hidden.

@bassosimone

bassosimone Sep 2, 2015

Member

Reading more carefully the header file, since connection is a transport, nothing really changed in this case since before. That is, connection used to be SharedPointer<Transport>. Now Transport (the abstract thing) is TransportInterface and Transport contains a SharedPointer<TransportInterface>. So, at the end of the day, here what really changed is that using transport is easier since the template is hidden.

char *family = NULL;
StringVector *pflist = NULL;
std::string address;
std::string port;

This comment has been minimized.

@hellais

hellais Sep 2, 2015

Contributor

Should port not be an int?

@hellais

hellais Sep 2, 2015

Contributor

Should port not be an int?

This comment has been minimized.

@bassosimone

bassosimone Sep 2, 2015

Member

At that point in the code it could be an integer as well as a string. In fact, later on the port value is passed to an overloaded function that could either receive the port as an integer or as a string.

Historically here there is a string because libneubot was designed also to be called from Python ctypes and moving checks in C++ simplified the glue code between Python and C++.

@bassosimone

bassosimone Sep 2, 2015

Member

At that point in the code it could be an integer as well as a string. In fact, later on the port value is passed to an overloaded function that could either receive the port as an integer or as a string.

Historically here there is a string because libneubot was designed also to be called from Python ctypes and moving checks in C++ simplified the glue code between Python and C++.

@hellais

This comment has been minimized.

Show comment
Hide comment
@hellais

hellais Sep 2, 2015

Contributor

I like 👍

A cursory glance does not lead me to suspect this has any issues in merging. Will review it with more detail and testing when I do #155.

As with that if for some reason I fail to do so by a reasonable time, proceed with merge.

Contributor

hellais commented Sep 2, 2015

I like 👍

A cursory glance does not lead me to suspect this has any issues in merging. Will review it with more detail and testing when I do #155.

As with that if for some reason I fail to do so by a reasonable time, proceed with merge.

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Sep 2, 2015

Member

Thanks!

Member

bassosimone commented Sep 2, 2015

Thanks!

@bassosimone bassosimone referenced this pull request Sep 4, 2015

Merged

Cleanup delayed call #162

@bassosimone

This comment has been minimized.

Show comment
Hide comment
@bassosimone

bassosimone Sep 4, 2015

Member

Merging!

Member

bassosimone commented Sep 4, 2015

Merging!

bassosimone added a commit that referenced this pull request Sep 4, 2015

@bassosimone bassosimone merged commit 218f35c into measurement-kit:master Sep 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bassosimone bassosimone deleted the bassosimone:feature/net-transport branch Sep 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment