code review 9782047: runtime: do not count type info in malloc stats

runtime: do not count type info in malloc stats
Otherwise testing.AllocsPerRun is flaky.
Fixes issue 5000.
Update issue 5525.

[dvyukov, 2013-05-28 16:32:36]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[dvyukov, 2013-05-28 16:33:53]

You can choose between this CL,
this CL: https://codereview.appspot.com/7625044/
and this CL: https://codereview.appspot.com/9837043

It's all an attempt to "fix" https://code.google.com/p/go/issues/detail?id=5000

[dvyukov, 2013-05-28 16:35:33]

On 2013/05/28 16:33:53, dvyukov wrote:
> You can choose between this CL,
> this CL: https://codereview.appspot.com/7625044/
> and this CL: https://codereview.appspot.com/9837043
> 
> It's all an attempt to "fix"
https://code.google.com/p/go/issues/detail?id=5000


I would prefer https://codereview.appspot.com/9837043
It effectively undoes my "persistentmalloc" CL, which removes ballast from heap,
thus making GCs more often (for very small heaps).
I suspect 9837043 can also significantly speedup very simple programs that
constantly compact heap from 100K to 50K.

[r, 2013-05-28 16:37:47]

AllocsPerRun is inherently flaky. There could be background stuff
going on we'll never control. Perhaps we should instead write our
tests not to be so sensitive (and build-breaking). I suggest all
AllocsPerRun tests should be skipped if -test.short is true, so they
are evaluated by the test owners but not in every build configuration
in the world.

-rob

[minux1, 2013-05-28 18:56:18]

On Wed, May 29, 2013 at 12:37 AM, Rob Pike <r@golang.org> wrote:

> AllocsPerRun is inherently flaky. There could be background stuff
> going on we'll never control. Perhaps we should instead write our
> tests not to be so sensitive (and build-breaking). I suggest all
> AllocsPerRun tests should be skipped if -test.short is true, so they
> are evaluated by the test owners but not in every build configuration
> in the world.
>
it follows that AllocsPerRun is badly designed API.
but we do need a way to reliably test malloc counts, if AllocPerRun is
flaky, we should still fix it.

i.e. we shouldn't add an API that's too flaky to be used in -short mode.

[dvyukov, 2013-05-28 18:57:57]

On Tue, May 28, 2013 at 10:55 PM, minux <minux.ma@gmail.com> wrote:

>
> On Wed, May 29, 2013 at 12:37 AM, Rob Pike <r@golang.org> wrote:
>
>> AllocsPerRun is inherently flaky. There could be background stuff
>> going on we'll never control. Perhaps we should instead write our
>> tests not to be so sensitive (and build-breaking). I suggest all
>> AllocsPerRun tests should be skipped if -test.short is true, so they
>> are evaluated by the test owners but not in every build configuration
>> in the world.
>>
> it follows that AllocsPerRun is badly designed API.
> but we do need a way to reliably test malloc counts, if AllocPerRun is
> flaky, we should still fix it.
>
> i.e. we shouldn't add an API that's too flaky to be used in -short mode.
>


We can eliminate the main current source flakyness in long run.

[minux1, 2013-05-28 19:05:42]

On Wed, May 29, 2013 at 2:57 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Tue, May 28, 2013 at 10:55 PM, minux <minux.ma@gmail.com> wrote:
>
>> On Wed, May 29, 2013 at 12:37 AM, Rob Pike <r@golang.org> wrote:
>>
>>> AllocsPerRun is inherently flaky. There could be background stuff
>>> going on we'll never control. Perhaps we should instead write our
>>> tests not to be so sensitive (and build-breaking). I suggest all
>>> AllocsPerRun tests should be skipped if -test.short is true, so they
>>> are evaluated by the test owners but not in every build configuration
>>> in the world.
>>>
>> it follows that AllocsPerRun is badly designed API.
>> but we do need a way to reliably test malloc counts, if AllocPerRun is
>> flaky, we should still fix it.
>>
>> i.e. we shouldn't add an API that's too flaky to be used in -short mode.
>>
> We can eliminate the main current source flakyness in long run.
>
If it's the case, i vote for keeping the current AllocsPerRun tests as is
and
aim to remove the (current) source of flakyness in the future.

if AllocsPerRun is flaky in -short mode, why won't it be flaky in non -short
mode? If it's always flaky, the API is broken as it can't be reliably used.

could we change AllocsPerRun to remove the flakyness?
for example, run more than 1 times to warm up?
disable GC during run?

[rsc, 2013-05-28 21:19:44]

Memory allocation stats should not report internal bookkeeping, so I agree
with the spirit of this CL. However, I don't understand the actual changes.
Can you make it just not include the bookkeeping from the start instead of
subtracting it out later?

It might be nice to make AllocsPerRun round the result to an integer (even
though it must return a float64). I regret not complaining harder about the
float64 result before Go 1.1.

Russ

[r, 2013-05-28 21:25:34]

I propose to fix AllocsPerRun to quantize the float64 to an integer
value before returning it, so if you get 1.01 as a result, it becomes
1 and
  if AllocsPerRun() == 1
works, not just
  if AllocsPerRun() < 2

But let me do that in a separate CL.

-rob

[dvyukov, 2013-05-29 07:44:02]

On 2013/05/28 21:19:44, rsc wrote:
> Memory allocation stats should not report internal bookkeeping, so I agree
> with the spirit of this CL. However, I don't understand the actual changes.
> Can you make it just not include the bookkeeping from the start instead of
> subtracting it out later?


Do you mean passing FlagNoStats flag to mallocgc/free? 2 reasons:
1. We don't knon when free happens, it's impossible to call free directly
because it will lead to several cases of recursion/deadlock, so it's freed by
GC.
2. There is another pending CL (https://codereview.appspot.com/9776043/) that
removes explicit stats collection, instead stats are derived from current heap
state. This cuts 6% from malloc as now, when other parts will become faster it
will be 12%, which means 2% end-to-end speedup for real apps (vtocc).

[dvyukov, 2013-05-29 07:45:39]

On 2013/05/28 21:25:34, r wrote:
> I propose to fix AllocsPerRun to quantize the float64 to an integer
> value before returning it, so if you get 1.01 as a result, it becomes
> 1 and
>   if AllocsPerRun() == 1
> works, not just
>   if AllocsPerRun() < 2
> 
> But let me do that in a separate CL.

FWIW, there are amortized algorithms that allocate not on each iteration (e.g.
hashmap), so allocation number can be 1/2 or 1/4.

[dave_cheney.net, 2013-05-29 09:07:15]

https://codereview.appspot.com/9782047/diff/6001/src/pkg/testing/allocs.go
File src/pkg/testing/allocs.go (right):

https://codereview.appspot.com/9782047/diff/6001/src/pkg/testing/allocs.go#ne...
src/pkg/testing/allocs.go:35: // This is need to collect type info blocks, see
issue 5000.
possibly reword this as

// This is needed to ensure type info blocks are collected prior to
ReadMemStats. See issue 5000.

^ assuming I interpreted the problem, and your solution correctly.

[dvyukov, 2013-05-31 06:39:23]

*** Abandoned ***