Re: [squid-dev] [RFC][PATCH] Bug4438 second attempt: give MemBlob its own pools

[Alex Rousskov]

On 03/16/2016 02:04 AM, Kinkie wrote:
> On Tue, Mar 15, 2016 at 9:43 PM, Alex Rousskov wrote:
>> On 03/14/2016 02:11 PM, Kinkie wrote:
>>>   this second attempt at bug4438 tries a different approach: by giving
>>> MemBlob its own pools and having a hard initialization dependency in
>>> MemBlob's allocating function, instead of relying on memAllocString.

>> Forgive me if I missed how this decision was made earlier, but I do not
>> understand why we want to duplicate the existing memAllocString()-like
>> functionality and split a single set of raw memory pools shared by all
>> callers into two isolated sets?

> It is an attempt to sidestep initialization issues by having pools in
> the same translation unit as their user.


That does not compute for me, on two levels:

1. I do not see how having pools in the same translation unit as their
_direct_ user helps. It is the indirect users that create initialization
order problems and those users obviously cannot be located in the same
translation unit. For example, how would your solution help in this
situation:

MemBlobPoolsLiveHere.cc:

  ... your pools live here ...
  ... MemBlob allocating code lives here too ...

SBufUserFoo.cc

  #include 
  static const SBuf Foo("foo"); // needs initialized MemBlob pools

What will guarantee that MemBlob pools are initialized when Foo is created?


2. If something in your new code does guarantee that the example in #1
works, then why cannot the same guarantee be used for fixing
memAllocString() instead, without duplicating its functionality?


>> In other words, how did we get from "SIGSEGV in memFreeString" (i.e.,
>> bug 4438) to this? If your working (or perhaps already proven!) theory
>> is that memAllocString() does something wrong, why not fix
>> memAllocString() instead of duplicating it? I must be missing something
>> obvious here...

> Unfortunately we can't prove what is doing what wrong as we (or at
> least I) can't reproduce the bug.

That cannot be it because your are essentially rewriting
memAllocString() in a new location and, hence, effectively deciding what
it is doing wrong (so that you do not copy [what you think is] the problem).

How about the following assertion? ```memAllocString() does not call
Mem::Init() when MemIsInitialized is false but still allocates memory
(which may later be freed) using difficult-to-follow logic that leads to
known minor and suspected big problems. This is wrong.'''

If that assertion is correct, and if that is the part of
memAllocString() that you are avoiding when duplicating its overall
functionality, then I think it would be better to fix memAllocString()
instead.


>>> - as no MemBlob can be initialized before MemPools, there is no need
>>> to baloon statically-initialized MemBlobs to
>>> SmallestStringBeforeMemIsInitialized.
>>
>> If your change guarantees that no MemBlob can be initialized before
>> MemPools, why cannot a similar change be done to the original code so
>> that no memAllocString()/etc. caller can get its result before MemPools
>> are initialized? AFAICT, you have solved the problem that you were
>> trying to solve, but I do not understand why you had to solve it in a
>> new location (leaving the old problem unsolved?).

> I do not yet know if I have solved the problem, unfortunately.
> If this attempt is successful, it can be a blueprint for modernizing
> memAllocString, or splitting other callers altogether. The file name
> (old_api) is to me an indicator that these API are not really
> encouraged, and on the way to deprecation if not deprecated already.

The approach of copying suspected-to-be-problematic code and then fixing
the copy does not make sense to me in this context, regardless of how
[not] confident you are that you have identified the problem correctly.

Furthermore, if the solution to the problem is placing pools in the same
translation unit as their direct user, then either I do not understand
the problem you are trying to solve, or you are not solving it (because
you have to worry about the _indirect_ users).


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC][PATCH] Bug4438 second attempt: give MemBlob its own pools

[Kinkie]

On Tue, Mar 15, 2016 at 9:43 PM, Alex Rousskov
 wrote:
> On 03/14/2016 02:11 PM, Kinkie wrote:
>
>>   this second attempt at bug4438 tries a different approach: by giving
>> MemBlob its own pools and having a hard initialization dependency in
>> MemBlob's allocating function, instead of relying on memAllocString.
>
> Forgive me if I missed how this decision was made earlier, but I do not
> understand why we want to duplicate the existing memAllocString()-like
> functionality and split a single set of raw memory pools shared by all
> callers into two isolated sets?

It is an attempt to sidestep initialization issues by having pools in
the same translation unit as their user.
The other effects are IMO generally beneficial but are not the main goal.

> In other words, how did we get from "SIGSEGV in memFreeString" (i.e.,
> bug 4438) to this? If your working (or perhaps already proven!) theory
> is that memAllocString() does something wrong, why not fix
> memAllocString() instead of duplicating it? I must be missing something
> obvious here...

Unfortunately we can't prove what is doing what wrong as we (or at
least I) can't reproduce the bug.

>> Known side-effects:
>> - allows to tune memblob sizes according to actual use patterns, which
>> can be collected via SBuf/MemBlob detailed stats. The currently-set
>> sizes follow some light testing I've done.
>
> Why is that a side effect? Could we not "tune memblob sizes according to
> actual use patterns" before this change?

We do not know the useage patterns of all memAllocString callers; we
do know those of MemBlob via SBufDetailedStats. As an alternative
approach for this kind of optimization, we could instrument
memAllocString to gather that data as well.

> Or do you meant that we can tune two separate sets of pools now? If yes,
> then you should also list "two separate sets of pools instead of one
> shared by all callers" as a side effect -- "more isolated sets" is the
> "cost" associated with this ability to tune more sets...

Nod.

>> - as no MemBlob can be initialized before MemPools, there is no need
>> to baloon statically-initialized MemBlobs to
>> SmallestStringBeforeMemIsInitialized.
>
> If your change guarantees that no MemBlob can be initialized before
> MemPools, why cannot a similar change be done to the original code so
> that no memAllocString()/etc. caller can get its result before MemPools
> are initialized? AFAICT, you have solved the problem that you were
> trying to solve, but I do not understand why you had to solve it in a
> new location (leaving the old problem unsolved?).

I do not yet know if I have solved the problem, unfortunately.
If this attempt is successful, it can be a blueprint for modernizing
memAllocString, or splitting other callers altogether. The file name
(old_api) is to me an indicator that these API are not really
encouraged, and on the way to deprecation if not deprecated already.

Thanks

-- 
Francesco
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC][PATCH] Bug4438 second attempt: give MemBlob its own pools

[Alex Rousskov]

On 03/14/2016 02:11 PM, Kinkie wrote:

>   this second attempt at bug4438 tries a different approach: by giving
> MemBlob its own pools and having a hard initialization dependency in
> MemBlob's allocating function, instead of relying on memAllocString.

Forgive me if I missed how this decision was made earlier, but I do not
understand why we want to duplicate the existing memAllocString()-like
functionality and split a single set of raw memory pools shared by all
callers into two isolated sets?

In other words, how did we get from "SIGSEGV in memFreeString" (i.e.,
bug 4438) to this? If your working (or perhaps already proven!) theory
is that memAllocString() does something wrong, why not fix
memAllocString() instead of duplicating it? I must be missing something
obvious here...


> Known side-effects:
> - allows to tune memblob sizes according to actual use patterns, which
> can be collected via SBuf/MemBlob detailed stats. The currently-set
> sizes follow some light testing I've done.

Why is that a side effect? Could we not "tune memblob sizes according to
actual use patterns" before this change?

Or do you meant that we can tune two separate sets of pools now? If yes,
then you should also list "two separate sets of pools instead of one
shared by all callers" as a side effect -- "more isolated sets" is the
"cost" associated with this ability to tune more sets...


> - as no MemBlob can be initialized before MemPools, there is no need
> to baloon statically-initialized MemBlobs to
> SmallestStringBeforeMemIsInitialized.

If your change guarantees that no MemBlob can be initialized before
MemPools, why cannot a similar change be done to the original code so
that no memAllocString()/etc. caller can get its result before MemPools
are initialized? AFAICT, you have solved the problem that you were
trying to solve, but I do not understand why you had to solve it in a
new location (leaving the old problem unsolved?).


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev