Re: git commit: TS-2415: use standard continuations to release UrlRewrite objects

[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

[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

[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

[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

[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

[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

[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