You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jp...@apache.org on 2013/12/04 06:30:32 UTC
git commit: TS-2415: use standard continuations to release UrlRewrite
objects
Updated Branches:
refs/heads/master 2b6a5f6bb -> 0f9305dc6
TS-2415: use standard continuations to release UrlRewrite objects
UrlRewrite uses a custom continuation to release the old object
after it is no longer needed. We can use library code for this
instead. Use new_Deleter to free the old UrlRewrite object.
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/0f9305dc
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/0f9305dc
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/0f9305dc
Branch: refs/heads/master
Commit: 0f9305dc685ee9cc7aa4dfca10afda3fb1a1ffb7
Parents: 2b6a5f6
Author: James Peach <jp...@apache.org>
Authored: Tue Dec 3 11:34:22 2013 -0800
Committer: James Peach <jp...@apache.org>
Committed: Tue Dec 3 21:29:02 2013 -0800
----------------------------------------------------------------------
CHANGES | 2 ++
proxy/ReverseProxy.cc | 22 +---------------------
2 files changed, 3 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0f9305dc/CHANGES
----------------------------------------------------------------------
diff --git a/CHANGES b/CHANGES
index fa8bb06..bac07c9 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,8 @@
Changes with Apache Traffic Server 4.2.0
+ *) [TS-2415] Use standard continuations to release UrlRewrite objects.
+
*) [TS-2413] Release memory for idle SSL connections.
Author: Wei Sun <su...@yahoo-inc.com>
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/0f9305dc/proxy/ReverseProxy.cc
----------------------------------------------------------------------
diff --git a/proxy/ReverseProxy.cc b/proxy/ReverseProxy.cc
index 51c3664..bdfecbd 100644
--- a/proxy/ReverseProxy.cc
+++ b/proxy/ReverseProxy.cc
@@ -146,26 +146,6 @@ struct UR_UpdateContinuation: public Continuation
}
};
-struct UR_FreerContinuation;
-typedef int (UR_FreerContinuation::*UR_FreerContHandler) (int, void *);
-
-/** Used to free url rewrite class. */
-struct UR_FreerContinuation: public Continuation
-{
- UrlRewrite *p;
- int freeEvent(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
- {
- Debug("url_rewrite", "Deleting old remap.config table");
- delete p;
- delete this;
- return EVENT_DONE;
- }
- UR_FreerContinuation(UrlRewrite * ap):Continuation(new_ProxyMutex()), p(ap)
- {
- SET_HANDLER((UR_FreerContHandler) & UR_FreerContinuation::freeEvent);
- }
-};
-
/**
Called when the remap.config file changes. Since it called infrequently,
we do the load of new file as blocking I/O and lock aquire is also
@@ -180,7 +160,7 @@ reloadUrlRewrite()
Debug("url_rewrite", "remap.config updated, reloading...");
newTable = NEW(new UrlRewrite());
if (newTable->is_valid()) {
- eventProcessor.schedule_in(new UR_FreerContinuation(rewrite_table), URL_REWRITE_TIMEOUT, ET_TASK);
+ new_Deleter(rewrite_table, URL_REWRITE_TIMEOUT);
Debug("url_rewrite", "remap.config done reloading!");
ink_atomic_swap(&rewrite_table, newTable);
} else {
Re: git commit: TS-2415: use standard continuations to release
UrlRewrite objects
Posted by James Peach <jp...@apache.org>.
On Dec 4, 2013, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
> On Dec 4, 2013, at 9:11 AM, James Peach <jp...@apache.org> wrote:
>
>> On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
>>
>>> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>>>>
>>>
>>>
>>> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>>>
>>> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
>>
>> Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
>
> Maybe we should move those config processors too ?
Yup.
>>> Maybe the right thing is to make new_Deleter use ET_TASK?
>>
>> Yes this sounds like the right thing to do.
>
> Ok, I’ll file a Jira (unless you already did?).
thanks
>>> If there are no task threads configured, that falls back to the old behavior?
>>
>> Or make sure we always have at least 1 task thread?
>
>
> Btw, that last sentence was a statement, not a question :). We do fall back on ET_NET (aka CALL) when there are no task threads.
>
>
> The default is at least 1 I think. We could probably change the behavior for v5.0 to require at least 1. Another Jira?
IMHO it would be a compatible change to make proxy.config.task_thread=0 mean "choose a sane default".
J
Re: git commit: TS-2415: use standard continuations to release
UrlRewrite objects
Posted by James Peach <jp...@apache.org>.
On Dec 4, 2013, at 2:04 PM, Leif Hedstrom <zw...@apache.org> wrote:
> On Dec 4, 2013, at 9:11 AM, James Peach <jp...@apache.org> wrote:
>
>> On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
>>
>>> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>>>>
>>>
>>>
>>> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>>>
>>> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
>>
>> Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
>
> Maybe we should move those config processors too ?
Yup.
>>> Maybe the right thing is to make new_Deleter use ET_TASK?
>>
>> Yes this sounds like the right thing to do.
>
> Ok, I’ll file a Jira (unless you already did?).
thanks
>>> If there are no task threads configured, that falls back to the old behavior?
>>
>> Or make sure we always have at least 1 task thread?
>
>
> Btw, that last sentence was a statement, not a question :). We do fall back on ET_NET (aka CALL) when there are no task threads.
>
>
> The default is at least 1 I think. We could probably change the behavior for v5.0 to require at least 1. Another Jira?
IMHO it would be a compatible change to make proxy.config.task_thread=0 mean "choose a sane default".
J
Re: git commit: TS-2415: use standard continuations to release UrlRewrite objects
Posted by Leif Hedstrom <zw...@apache.org>.
On Dec 4, 2013, at 9:11 AM, James Peach <jp...@apache.org> wrote:
> On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
>
>> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>>>
>>
>>
>> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>>
>> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
>
> Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
Maybe we should move those config processors too ?
>
>> Maybe the right thing is to make new_Deleter use ET_TASK?
>
> Yes this sounds like the right thing to do.
Ok, I’ll file a Jira (unless you already did?).
>
>> If there are no task threads configured, that falls back to the old behavior?
>
> Or make sure we always have at least 1 task thread?
Btw, that last sentence was a statement, not a question :). We do fall back on ET_NET (aka CALL) when there are no task threads.
The default is at least 1 I think. We could probably change the behavior for v5.0 to require at least 1. Another Jira?
— Leif
Re: git commit: TS-2415: use standard continuations to release UrlRewrite objects
Posted by Leif Hedstrom <zw...@apache.org>.
On Dec 4, 2013, at 9:11 AM, James Peach <jp...@apache.org> wrote:
> On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
>
>> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>>>
>>
>>
>> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>>
>> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
>
> Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
Maybe we should move those config processors too ?
>
>> Maybe the right thing is to make new_Deleter use ET_TASK?
>
> Yes this sounds like the right thing to do.
Ok, I’ll file a Jira (unless you already did?).
>
>> If there are no task threads configured, that falls back to the old behavior?
>
> Or make sure we always have at least 1 task thread?
Btw, that last sentence was a statement, not a question :). We do fall back on ET_NET (aka CALL) when there are no task threads.
The default is at least 1 I think. We could probably change the behavior for v5.0 to require at least 1. Another Jira?
— Leif
Re: git commit: TS-2415: use standard continuations to release
UrlRewrite objects
Posted by James Peach <jp...@apache.org>.
On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>
>> Updated Branches:
>> refs/heads/master 2b6a5f6bb -> 0f9305dc6
>>
>>
>> TS-2415: use standard continuations to release UrlRewrite objects
>>
>> UrlRewrite uses a custom continuation to release the old object
>> after it is no longer needed. We can use library code for this
>> instead. Use new_Deleter to free the old UrlRewrite object.
>> newTable = NEW(new UrlRewrite());
>> if (newTable->is_valid()) {
>> - eventProcessor.schedule_in(new UR_FreerContinuation(rewrite_table), URL_REWRITE_TIMEOUT, ET_TASK);
>> + new_Deleter(rewrite_table, URL_REWRITE_TIMEOUT);
>> Debug("url_rewrite", "remap.config done reloading!");
>> ink_atomic_swap(&rewrite_table, newTable);
>> } else {
>>
>
>
> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>
> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
> Maybe the right thing is to make new_Deleter use ET_TASK?
Yes this sounds like the right thing to do.
> If there are no task threads configured, that falls back to the old behavior?
Or make sure we always have at least 1 task thread?
J
Re: git commit: TS-2415: use standard continuations to release
UrlRewrite objects
Posted by James Peach <jp...@apache.org>.
On Dec 4, 2013, at 7:37 AM, Leif Hedstrom <zw...@apache.org> wrote:
> On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
>
>> Updated Branches:
>> refs/heads/master 2b6a5f6bb -> 0f9305dc6
>>
>>
>> TS-2415: use standard continuations to release UrlRewrite objects
>>
>> UrlRewrite uses a custom continuation to release the old object
>> after it is no longer needed. We can use library code for this
>> instead. Use new_Deleter to free the old UrlRewrite object.
>> newTable = NEW(new UrlRewrite());
>> if (newTable->is_valid()) {
>> - eventProcessor.schedule_in(new UR_FreerContinuation(rewrite_table), URL_REWRITE_TIMEOUT, ET_TASK);
>> + new_Deleter(rewrite_table, URL_REWRITE_TIMEOUT);
>> Debug("url_rewrite", "remap.config done reloading!");
>> ink_atomic_swap(&rewrite_table, newTable);
>> } else {
>>
>
>
> Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
>
> I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
Oh interesting. All the config processor stuff happens on ET_CALL threads too. The remap.config loading still happens on the ET_TASK.
> Maybe the right thing is to make new_Deleter use ET_TASK?
Yes this sounds like the right thing to do.
> If there are no task threads configured, that falls back to the old behavior?
Or make sure we always have at least 1 task thread?
J
Re: git commit: TS-2415: use standard continuations to release UrlRewrite objects
Posted by Leif Hedstrom <zw...@apache.org>.
On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
> Updated Branches:
> refs/heads/master 2b6a5f6bb -> 0f9305dc6
>
>
> TS-2415: use standard continuations to release UrlRewrite objects
>
> UrlRewrite uses a custom continuation to release the old object
> after it is no longer needed. We can use library code for this
> instead. Use new_Deleter to free the old UrlRewrite object.
> newTable = NEW(new UrlRewrite());
> if (newTable->is_valid()) {
> - eventProcessor.schedule_in(new UR_FreerContinuation(rewrite_table), URL_REWRITE_TIMEOUT, ET_TASK);
> + new_Deleter(rewrite_table, URL_REWRITE_TIMEOUT);
> Debug("url_rewrite", "remap.config done reloading!");
> ink_atomic_swap(&rewrite_table, newTable);
> } else {
>
Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
Maybe the right thing is to make new_Deleter use ET_TASK? If there are no task threads configured, that falls back to the old behavior?
— Leif
Re: git commit: TS-2415: use standard continuations to release UrlRewrite objects
Posted by Leif Hedstrom <zw...@apache.org>.
On Dec 3, 2013, at 10:30 PM, jpeach@apache.org wrote:
> Updated Branches:
> refs/heads/master 2b6a5f6bb -> 0f9305dc6
>
>
> TS-2415: use standard continuations to release UrlRewrite objects
>
> UrlRewrite uses a custom continuation to release the old object
> after it is no longer needed. We can use library code for this
> instead. Use new_Deleter to free the old UrlRewrite object.
> newTable = NEW(new UrlRewrite());
> if (newTable->is_valid()) {
> - eventProcessor.schedule_in(new UR_FreerContinuation(rewrite_table), URL_REWRITE_TIMEOUT, ET_TASK);
> + new_Deleter(rewrite_table, URL_REWRITE_TIMEOUT);
> Debug("url_rewrite", "remap.config done reloading!");
> ink_atomic_swap(&rewrite_table, newTable);
> } else {
>
Fwiw, this new_Deleter does not have the same semantics as the old code. The old code would schedule the deletion on the ET_TASK thread pool, whereas new_Deleter runs on any of the net-threads. This could mean that we’d block a net-thread for some time if the remap table is sufficiently large.
I don’t know if it is a real, detrimental problem, but if I recall we moved the remap.config reloading / management to ET_TASK because it did have a real problem when loading very large configs.
Maybe the right thing is to make new_Deleter use ET_TASK? If there are no task threads configured, that falls back to the old behavior?
— Leif