You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by ganeshmurthy <gi...@git.apache.org> on 2018/08/10 01:46:34 UTC
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
GitHub user ganeshmurthy opened a pull request:
https://github.com/apache/qpid-dispatch/pull/357
DISPATCH-1099 - Added plumbing that sends ticks into the core
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ganeshmurthy/qpid-dispatch DISPATCH-1099-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-dispatch/pull/357.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #357
----
commit e6a04c338bca921290e2f774b82cf8302220c185
Author: Ganesh Murthy <gm...@...>
Date: 2018-08-09T17:26:03Z
DISPATCH-1099 - Added plumbing that sends ticks into the core
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #357: DISPATCH-1099 - Added plumbing that sends ticks in...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/qpid-dispatch/pull/357
# [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=h1) Report
> Merging [#357](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=desc) into [master](https://codecov.io/gh/apache/qpid-dispatch/commit/18c73aeed0e0ae906eea3a903121c03bec815088?src=pr&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `28.57%`.
[](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #357 +/- ##
=========================================
- Coverage 84.5% 84.41% -0.1%
=========================================
Files 69 70 +1
Lines 15729 15758 +29
=========================================
+ Hits 13292 13302 +10
- Misses 2437 2456 +19
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `98.9% <0%> (-0.37%)` | :arrow_down: |
| [src/router\_core/core\_timer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL2NvcmVfdGltZXIuYw==) | `0% <0%> (ø)` | |
| [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `93.52% <100%> (ø)` | :arrow_up: |
| [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `89.05% <36.84%> (-1.51%)` | :arrow_down: |
| [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL21lc3NhZ2UuYw==) | `88% <0%> (+0.01%)` | :arrow_up: |
| [src/router\_core/agent\_link.c](https://codecov.io/gh/apache/qpid-dispatch/pull/357/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL2FnZW50X2xpbmsuYw==) | `63.79% <0%> (+0.57%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=footer). Last update [18c73ae...e6a04c3](https://codecov.io/gh/apache/qpid-dispatch/pull/357?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209298303
--- Diff: src/router_core/transfer.c ---
@@ -717,6 +725,35 @@ void qdr_delivery_decref_CT(qdr_core_t *core, qdr_delivery_t *dlv, const char *l
qdr_delete_delivery_internal_CT(core, dlv);
}
+static void qdr_process_tick_CT(qdr_core_t *core, qdr_action_t *action, bool discard)
+{
+ if (discard)
+ return;
+
+ if (DEQ_SIZE(core->timer_list) == 0)
+ return;
+
+ qdr_timer_work_t *next_timer_work = 0;
+
+ qdr_timer_work_t *timer_work = DEQ_HEAD(core->timer_list);
+
+ while (timer_work) {
+ if (timer_work->timer_delay == 0) { // It is time to execute the handler
+
+ timer_work->handler(core, timer_work->on_timer_context);
+ next_timer_work = DEQ_NEXT(timer_work);
+ DEQ_REMOVE(core->timer_list, timer_work);
+ free_qdr_timer_work_t(timer_work);
+ timer_work = next_timer_work;
+ }
+ else {
+ timer_work->timer_delay -=1;
+ timer_work = DEQ_NEXT(timer_work);
+ }
+
--- End diff --
As stated in another comment: This algorithm is going to be costly when there are a large number of timers in the list. The tick handler must visit every timer on every tick.
If the list were ordered by duration and the time stored in the timer_work were relative (i.e. number of ticks after the previous work), then the tick handler only needs to visit the head of the list.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209296711
--- Diff: src/router_core/core_timer.c ---
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "router_core_private.h"
+#include <stdio.h>
+
+qdr_timer_work_t *qdr_timer_schedule(qdr_core_t *core, qdr_timer_cb_t callback, void *timer_context, int timer_delay)
+{
+ qdr_timer_work_t *timer_work = new_qdr_timer_work_t();
+ ZERO(timer_work);
+ timer_work->handler = callback;
+ timer_work->timer_delay = timer_delay;
+ timer_work->on_timer_context = timer_context;
+
+ DEQ_INSERT_TAIL(core->timer_list, timer_work);
--- End diff --
The timer algorithm should be reviewed, I think. This at-tail insertion with absolute delay is going to be very inefficient if there are a large number of timers (which is a probably scenario for cases where there are many auto-links).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209165667
--- Diff: src/router_core/core_timer.c ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "router_core_private.h"
+
+
+void qdr_timner_schedule(qdr_core_t *core, qdr_timer_cb_t callback, void *timer_context, int timer_delay)
--- End diff --
s/timner/timer/ ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #357: DISPATCH-1099 - Added plumbing that sends ticks in...
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on the issue:
https://github.com/apache/qpid-dispatch/pull/357
It adds one action record into the action list every one second. That should not bring down the performance but I will try using quiver and see what I get
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209297255
--- Diff: src/router_core/transfer.c ---
@@ -717,6 +725,35 @@ void qdr_delivery_decref_CT(qdr_core_t *core, qdr_delivery_t *dlv, const char *l
qdr_delete_delivery_internal_CT(core, dlv);
}
+static void qdr_process_tick_CT(qdr_core_t *core, qdr_action_t *action, bool discard)
+{
+ if (discard)
+ return;
+
+ if (DEQ_SIZE(core->timer_list) == 0)
+ return;
--- End diff --
This check is not needed. If the timer list is empty, the while loop below will exit immediately.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209304945
--- Diff: include/qpid/dispatch/router_core.h ---
@@ -797,4 +799,9 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted,
int ssl_ssf,
bool ssl);
+typedef struct qdr_timer_work_t qdr_timer_work_t;
+typedef void (*qdr_timer_cb_t)(qdr_core_t *core, void* context);
+qdr_timer_work_t *qdr_timer_schedule(qdr_core_t *core, qdr_timer_cb_t callback, void *timer_context, int timer_delay);
+void qdr_timer_delete(qdr_core_t *core, qdr_timer_work_t *timer_work);
--- End diff --
Other timer APIs we've created look more like this:
qdr_core_timer_t *qdr_core_timer();
void qdr_core_timer_schedule(qdr_core_timer_t *timer, ...);
void qdr_core_timer_cancel(qdr_core_timer_t *timer);
void qdr_core_timer_free(qdr_core_timer_t *timer);
This prevents you from having to create/allocate a new timer each time you reschedule. The timers we will be using will be recurring, so this more classic API fits well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy closed the pull request at:
https://github.com/apache/qpid-dispatch/pull/357
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209295809
--- Diff: include/qpid/dispatch/router_core.h ---
@@ -797,4 +799,9 @@ qdr_connection_info_t *qdr_connection_info(bool is_encrypted,
int ssl_ssf,
bool ssl);
+typedef struct qdr_timer_work_t qdr_timer_work_t;
+typedef void (*qdr_timer_cb_t)(qdr_core_t *core, void* context);
+qdr_timer_work_t *qdr_timer_schedule(qdr_core_t *core, qdr_timer_cb_t callback, void *timer_context, int timer_delay);
+void qdr_timer_delete(qdr_core_t *core, qdr_timer_work_t *timer_work);
--- End diff --
Some comments on these function prototypes would be helpful? What exactly does qdr_timer_delete do? Should there be a qdr_timer_cancel()?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #357: DISPATCH-1099 - Added plumbing that sends ticks in...
Posted by grs <gi...@git.apache.org>.
Github user grs commented on the issue:
https://github.com/apache/qpid-dispatch/pull/357
Possibly silly question, but is there any performance impact to this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #357: DISPATCH-1099 - Added plumbing that sends ticks in...
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on the issue:
https://github.com/apache/qpid-dispatch/pull/357
Sorry, also depends on the number of outstanding timers. At this time, we plan to add timers for retrying autoLinks only. I will add a bunch of autoLinks that need to be retried and see how it impacts performance.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #357: DISPATCH-1099 - Added plumbing that sends t...
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/357#discussion_r209298628
--- Diff: src/router_node.c ---
@@ -1179,6 +1180,9 @@ static void qd_router_timer_handler(void *context)
// Periodic processing.
//
qd_pyrouter_tick(router);
+
+ qdr_process_tick(router->router_core);
+
--- End diff --
Nitpick: Why so much whitespace between these lines?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org