simsong
open_flows.erase called even if file is not opened.
If a flow has nothing to write, its tcp->fd is never set and it is not inserted in demux.open_flows. but in tcpip::close_file(), demux.open_flows.erase(this) is called even if fd is not set. Is it intended? I'm replacing current 'close_oldest_fd with open_flows unordered_set' code with my code. and I assumed that when 'open_flows.erase(this)' is called, 'this' is always in the open_flows set. but it seems not.
What is your code? What is tcp->fd when it is not set? tcpip::close_file() should probably check to make sure that the file it is closing is opened, but demux.open_flows.erase(this) should run in any case, because the flow needs to be removed.
On Sep 30, 2015, at 8:02 AM, EaseTheWorld [email protected] wrote:
If a flow has nothing to write, its tcp->fd is never set and it is not inserted in demux.open_flows. but in tcpip::close_file(), demux.open_flows.erase(this) is called even if fd is not set. Is it intended? I'm replacing current 'close_oldest_fd with open_flows unordered_set' code with my code. and I assumed that when 'open_flows.erase(this)' is called, 'this' is always in the open_flows set. but it seems not.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109.
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
Okay. You are more familiar with the code than I am at the moment, then. Are you manually implementing the linked list, or are you going to use a c++ priority_queue?
On Sep 30, 2015, at 8:44 AM, EaseTheWorld [email protected] wrote:
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144385139.
Manually implemented double linked list. To implement 'add', 'move to end', 'remove' at constant time, tcpip class itself has to keep *prev, *next and tcpdemux manages the list.
2015-09-30 21:48 GMT+09:00 Simson L. Garfinkel [email protected]:
Okay. You are more familiar with the code than I am at the moment, then. Are you manually implementing the linked list, or are you going to use a c++ priority_queue?
On Sep 30, 2015, at 8:44 AM, EaseTheWorld [email protected] wrote:
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144385139>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144386062.
Isn't it better to use existing classes?
On Sep 30, 2015, at 9:29 AM, EaseTheWorld [email protected] wrote:
Manually implemented double linked list. To implement 'add', 'move to end', 'remove' at constant time, tcpip class itself has to keep *prev, *next and tcpdemux manages the list.
2015-09-30 21:48 GMT+09:00 Simson L. Garfinkel [email protected]:
Okay. You are more familiar with the code than I am at the moment, then. Are you manually implementing the linked list, or are you going to use a c++ priority_queue?
On Sep 30, 2015, at 8:44 AM, EaseTheWorld [email protected] wrote:
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144385139>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144386062.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144396524.
I agree but I couldn't find a proper one.
- When a packet arrives, the matching tcpip should move(or add) to the end of the list without linear search.
- When close the oldest, the list should return the first(oldest one). Any ideas are welcome :)
-
-
- 오후 10:49에 "Simson L. Garfinkel" [email protected]님이 작성:
-
Isn't it better to use existing classes?
On Sep 30, 2015, at 9:29 AM, EaseTheWorld [email protected] wrote:
Manually implemented double linked list. To implement 'add', 'move to end', 'remove' at constant time, tcpip class itself has to keep *prev, *next and tcpdemux manages the list.
2015-09-30 21:48 GMT+09:00 Simson L. Garfinkel <[email protected] :
Okay. You are more familiar with the code than I am at the moment, then. Are you manually implementing the linked list, or are you going to use a c++ priority_queue?
On Sep 30, 2015, at 8:44 AM, EaseTheWorld [email protected] wrote:
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144385139>.
— Reply to this email directly or view it on GitHub <https://github.com/simsong/tcpflow/issues/109#issuecomment-144386062 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144396524>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144406205.
If the C++ STL priority queue implementation didn't work, then you can go with your own. It's not that complicated. But I thought that the priority_queue would do this.
On Wed, Sep 30, 2015 at 9:58 AM, EaseTheWorld [email protected] wrote:
I agree but I couldn't find a proper one.
- When a packet arrives, the matching tcpip should move(or add) to the end of the list without linear search.
- When close the oldest, the list should return the first(oldest one). Any ideas are welcome :)
- 오후 10:49에 "Simson L. Garfinkel" [email protected]님이 작성:
Isn't it better to use existing classes?
On Sep 30, 2015, at 9:29 AM, EaseTheWorld [email protected] wrote:
Manually implemented double linked list. To implement 'add', 'move to end', 'remove' at constant time, tcpip class itself has to keep *prev, *next and tcpdemux manages the list.
2015-09-30 21:48 GMT+09:00 Simson L. Garfinkel < [email protected] :
Okay. You are more familiar with the code than I am at the moment, then. Are you manually implementing the linked list, or are you going to use a c++ priority_queue?
On Sep 30, 2015, at 8:44 AM, EaseTheWorld < [email protected]> wrote:
in tcpip::open_file, open_flows.insert() is never called if fd < 0. which means open_flows.erase() shouldn't(doesn't have to) be called as well.
My code is about improvement of close_oldest_fd logic. which is currently iterate all open_flows to find the oldest. What I'm thinking of is a linked list of tcpip which always keeps in access order, so 'find the oldest', 'add', 'ordering' is in constant time. I need an assumption the list always erase elements which is existed.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144385139 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144386062 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144396524>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144406205.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144415019.
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld [email protected] wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" [email protected]님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld [email protected] wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315.
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld [email protected] wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" [email protected]님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld [email protected] wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057.
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" [email protected]님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld [email protected] wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" [email protected]님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld [email protected] wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub <https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026.
I thought that you could just remove the node, change the value as you want it, and then re-add it. Would that not work?
On Thu, Oct 1, 2015 at 9:55 AM, EaseTheWorld [email protected] wrote:
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" [email protected]님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld [email protected] wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" <[email protected] 님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld <[email protected]
wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144732651.
I don't change any value, only the order matters. and add(), remove(), size() should be constant time. I thought that you could just remove the node, change the value as you want it, and then re-add it. Would that not work?
On Thu, Oct 1, 2015 at 9:55 AM, EaseTheWorld [email protected] wrote:
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" [email protected]님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld [email protected] wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" <[email protected] 님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld <[email protected]
wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057>.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144732651.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144733602.
My idea was to use a priority queue, and have the sorting done with the priority, such as when it was last used.
On Thu, Oct 1, 2015 at 10:36 AM, EaseTheWorld [email protected] wrote:
I don't change any value, only the order matters. and add(), remove(), size() should be constant time.
I thought that you could just remove the node, change the value as you want it, and then re-add it. Would that not work?
On Thu, Oct 1, 2015 at 9:55 AM, EaseTheWorld [email protected] wrote:
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" [email protected]님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld [email protected] wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" < [email protected] 님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld < [email protected]
wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057>.
— Reply to this email directly or view it on GitHub <https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026 .
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144732651.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144733602.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144747106.
I see. Your idea is to sort last packet time with priority_queue and my idea is to move latest flow to the end with list. then I'll commit my idea (maybe a few days later..) so you can check. 2015. 10. 1. 오후 11:42에 "Simson L. Garfinkel" [email protected]님이 작성:
My idea was to use a priority queue, and have the sorting done with the priority, such as when it was last used.
On Thu, Oct 1, 2015 at 10:36 AM, EaseTheWorld [email protected] wrote:
I don't change any value, only the order matters. and add(), remove(), size() should be constant time.
I thought that you could just remove the node, change the value as you want it, and then re-add it. Would that not work?
On Thu, Oct 1, 2015 at 9:55 AM, EaseTheWorld [email protected] wrote:
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" <[email protected] 님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld <[email protected]
wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" < [email protected] 님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld < [email protected]
wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026 .
— Reply to this email directly or view it on GitHub <https://github.com/simsong/tcpflow/issues/109#issuecomment-144732651 .
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144733602.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144747106.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144748926.
That was my idea. However, if all you need is a doubly linked list, then you should should use the std::list. When stream is accessed remove the item from the list and add it to the head. Delete from the tail. You don't need a priority queue.
On Oct 1, 2015, at 5:57 PM, EaseTheWorld [email protected] wrote:
I see. Your idea is to sort last packet time with priority_queue and my idea is to move latest flow to the end with list. then I'll commit my idea (maybe a few days later..) so you can check. 2015. 10. 1. 오후 11:42에 "Simson L. Garfinkel" [email protected]님이 작성:
My idea was to use a priority queue, and have the sorting done with the priority, such as when it was last used.
On Thu, Oct 1, 2015 at 10:36 AM, EaseTheWorld [email protected] wrote:
I don't change any value, only the order matters. and add(), remove(), size() should be constant time.
I thought that you could just remove the node, change the value as you want it, and then re-add it. Would that not work?
On Thu, Oct 1, 2015 at 9:55 AM, EaseTheWorld [email protected] wrote:
I checked priority_queue, but I don't know how to move a node to the end.(i.e. when a packet is arrived, the flow should be latest.) and also remove the first(close oldest fd) element should be available. Is it possible with priority queue? 2015. 10. 1. 오후 7:42에 "Simson L. Garfinkel" <[email protected] 님이 작성:
Did you look at std::priority_queue()?
I don't know how many other headers boost::intrusive::list will include.
On Oct 1, 2015, at 6:01 AM, EaseTheWorld <[email protected]
wrote:
Oh. that is interesting. i didn't know boost until I see tcpflow source.
I'm gonna include one header <boost/Intrusive/list.hpp>.
Like you said I prefer using existed library rather than implementing myself. At first I tried std::list, it seemed to work but turned out list.size() is not constant time complexity so I chose boost::intrusive::list.
if you are still not comfortable with this, I'll use my implemented list. 2015. 10. 1. 오후 6:53에 "Simson L. Garfinkel" < [email protected] 님이 작성:
Please don't use boost in the final version. It creates a huge dependency. I've had very bad experience with boost. If you can use just a few include files and incorporate them, that is okay, but no boost dependencies...
Sent from my iPhone
On Oct 1, 2015, at 1:25 AM, EaseTheWorld < [email protected]
wrote:
I'm seeing performance improvement with my own linked-list implementation. but I think I can use boost::intrusive::list with same result. I'll try soon.
— Reply to this email directly or view it on GitHub.
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144682315 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144684057 .
— Reply to this email directly or view it on GitHub < https://github.com/simsong/tcpflow/issues/109#issuecomment-144691026 .
— Reply to this email directly or view it on GitHub <https://github.com/simsong/tcpflow/issues/109#issuecomment-144732651 .
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144733602.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144747106.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144748926.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-144861389.
I wanted to use std::list too but there are some issues.
- To remove(or splice) a item in constant time, I need an iterator as a field in tcpip class. Is it ok to keep an iterator?
- std::list.size() is O(n) on some platform including my Ubuntu 14.04. (boost::intrusive::list is always O(1)) If these two issues are resolved, I don't have to use boost::intrusive::list.
I don't think that there is a problem keeping an iterative. Why would there be?
Why is performance of size() different on Ubuntu 14.04? Old version of STL? If need be, can't we just keep track of the count manually?
Simson
Sent from my iPhone
On Oct 2, 2015, at 12:40 AM, EaseTheWorld [email protected] wrote:
I wanted to use std::list too but there are some issues.
- To remove(or splice) a item in constant time, I need an iterator as a field in tcpip class. Is it ok to keep an iterator?
- std::list.size() is O(n) on some platform including my Ubuntu 14.04. (boost::intrusive::list is always O(1)) If these two issues are resolved, I don't have to use boost::intrusive::list.
— Reply to this email directly or view it on GitHub.
You are right. I worried too much. std::list has a property that iterator is valid unless the item itself removed. http://stackoverflow.com/questions/837267/safe-to-store-listiterator-for-later-use
About complexity of list::size(), it is implementation-dependent. http://stackoverflow.com/questions/19154205/should-stdlistsize-have-constant-complexity-in-c11
But I found something interesting. In retrying_open(), "if (open_flows.size() >= max_fds) close_oldest_fd();" might not be necessary at all. I removed the line and compare the performance but the result is surprisingly almost same. Do you know any history about this? If this line is removed, then get_max_fds() code can be removed, too.
Some users wanted to restrict the number of open files below the operating system limit, hence max_fds.
Another way to implement it would be using setrlimit, I suppose
On Oct 4, 2015, at 8:05 PM, EaseTheWorld [email protected] wrote:
You are right. I worried too much. std::list has a property that iterator is valid unless the item itself removed. http://stackoverflow.com/questions/837267/safe-to-store-listiterator-for-later-use http://stackoverflow.com/questions/837267/safe-to-store-listiterator-for-later-use About complexity of list::size(), it is implementation-dependent. http://stackoverflow.com/questions/19154205/should-stdlistsize-have-constant-complexity-in-c11 http://stackoverflow.com/questions/19154205/should-stdlistsize-have-constant-complexity-in-c11 But I found something interesting. In retrying_open(), "if (open_flows.size() >= max_fds) close_oldest_fd();" might not be necessary at all. I removed the line and compare the performance but the result is surprisingly almost same. Do you know any history about this? If this line is removed, then get_max_fds() code can be removed, too.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-145403816.
OK then tcpdemux::get_max_fds() code can be removed anyway. and unless users restricts the number of open files, and open_flows.size() check is not necessary and even if users do restrict the number, setrlimit() is enough and open_flows.size() check is still not necessary. Do you agree?
Hi. No, I don't agree. Several users wanted to be able to restrict the max number of fds. It's useful code, and it's there at a user request. It's also incredibly useful for testing. Just leave it for now.
Thanks.
On Oct 4, 2015, at 9:56 PM, EaseTheWorld [email protected] wrote:
OK then tcpdemux::get_max_fds() code can be removed anyway. and unless users restricts the number of open files, and open_flows.size() check is not necessary and even if users do restrict the number, setrlimit() is enough and open_flows.size() check is still not necessary. Do you agree?
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-145410155.
I didn't mean to remove the 'restrict the max fd' feature. I meant tcpdemux::get_max_fds() code & open_flows.size() check is not necessary. because setrlimit(max_fd_by_user) and open errno (EMFILE, ENFILE) will do the job.
I don't know if setrlimit() would work. I haven't tested that, and I haven't discussed it with the potentially affected users.
On Oct 4, 2015, at 11:16 PM, EaseTheWorld [email protected] wrote:
I didn't mean to remove the 'restrict the max fd' feature. I meant tcpdemux::get_max_fds() code & open_flows.size() check is not necessary. because setrlimit(max_fd_by_user) and open errno (EMFILE, ENFILE) will do the job.
— Reply to this email directly or view it on GitHub https://github.com/simsong/tcpflow/issues/109#issuecomment-145419361.
If so, then open_flows.size() check is necessary to retrisct the max fd. and open_flows.size() is also used to log max_open_flows.xml I wanted to get rid of calling std::list.size() but it seems to be necessary in some feature. I'll try to keep the feature and improve performance as much as possible.
If I have no choice but to use boost::intrusive::list, I'll fork a repository.
Let me look at the code in depth.
Sent from my iPhone
On Oct 5, 2015, at 3:24 AM, EaseTheWorld [email protected] wrote:
If so, then open_flows.size() check is necessary to retrisct the max fd. and open_flows.size() is also used to log max_open_flows.xml I wanted to get rid of calling std::list.size() but it seems to be necessary in some feature. I'll try to keep the feature and improve performance as much as possible.
— Reply to this email directly or view it on GitHub.
It would be better to use #ifdef.
However, we can delete the option, rather than fork.
Sent from my iPhone
On Oct 5, 2015, at 3:28 AM, EaseTheWorld [email protected] wrote:
If I have no choice but to use boost::intrusive::list, I'll fork a repository.
— Reply to this email directly or view it on GitHub.
'PullRequest Improve close_oldest logic #111 ' Sorry, I was late. These days I've been busy for my wedding preparation... :)