Closed mpaolini closed 9 years ago
And also the previous implementation used channel_ids.pop(0)
for fetching a free channel id. This has O(n) complexity in python because list
is an array. collections.deque
would have been a better choice.
Thanks for this patch anyway !
we should be using self.server_channel_max
as it's used here https://github.com/Polyconseil/aioamqp/blob/master/aioamqp/protocol.py#L47 instead of the constant.
This helps if we want to call tune_ok() with custom parameters.
ok gave it a try... is this what you had in mind?
mh I think we should have an asyncio.Event set when new channel ids are available and let channel() "block" waiting the event to be set ?
Does the library should raise if the max_channel is reached or should the yield from protocol.channel() wait for a new channel to be available ?
@dzen I think for now raising an exception can do. It has always been like this in aioamqp and it is out of this patch's scope.
This patch is really only about fixing performance issues introduced in 0e094fe . I would like this patch merged so I can upgrade aioamqp and keep testing. Thanks ;)
ping
you're right, we still need to keep the same behavior for now.
LGTM.
Would you please rebase your branch and add an entry into the changelog ?
thank you.
will do this weekend.
OT: @dzen is it possible to setup CI and integrate it here on github? I can use some help. Let me know
Le vendredi 2 octobre 2015, Marco Paolini notifications@github.com a écrit :
will do this weekend.
OT: @dzen https://github.com/dzen is it possible to setup CI and integrate it here on github? I can use some help. Let me know
Hello,
Travis CI is already setup for aioamqp an it builds branches normally
— Reply to this email directly or view it on GitHub https://github.com/Polyconseil/aioamqp/pull/43#issuecomment-144937075.
Benoît CALVEZ Polyconseil | 26 rue de Berri | 75008 Paris
Here is the link to it. https://travis-ci.org/Polyconseil/aioamqp
Le vendredi 2 octobre 2015, Benoit Calvez benoit.calvez@polyconseil.fr a écrit :
Le vendredi 2 octobre 2015, Marco Paolini <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> a écrit :
will do this weekend.
OT: @dzen https://github.com/dzen is it possible to setup CI and integrate it here on github? I can use some help. Let me know
Hello,
Travis CI is already setup for aioamqp an it builds branches normally
— Reply to this email directly or view it on GitHub https://github.com/Polyconseil/aioamqp/pull/43#issuecomment-144937075.
Benoît CALVEZ Polyconseil | 26 rue de Berri | 75008 Paris
Benoît CALVEZ Polyconseil | 26 rue de Berri | 75008 Paris
how nice! can be integrated with github so we get the test results in the pull request here?
Yes, I'll look into int today :)
On Fri, Oct 2, 2015 at 9:32 AM, Marco Paolini notifications@github.com wrote:
how nice! can be integrated with github so we get the test results in the pull request here?
— Reply to this email directly or view it on GitHub https://github.com/Polyconseil/aioamqp/pull/43#issuecomment-144943268.
Benoît CALVEZ Polyconseil | 26 rue de Berri | 75008 Paris
Hello @mpaolini,
If you wish, your PR/branch is built there:
replaced by #49 that cleans up stray merges
0e094fe746a9a39f96b5fdc9d8c3874552d4c9d7 introduced reuse of channel id.
The implementation allocates a free list
channels_ids
holding all unused channel ids available. When the protocol is initialized the list contains 65535 ints, making up roughly half a MB.This new implementation instead uses a set() to track reclaimed channel ids. Whenever a new channel id is needed, we first try to take it from the free set(). If the free set() is empty we forge a new channel id incrementing a counter.