Re: [PATCH] dtrace probes for memory manager

--On 11. Dezember 2009 11:28:54 -0300 Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:

>>
>> without compiled probes: AVG(2531.68)
>> with compiled probes: AVG(2511.97)
>
> Were the probes enabled?

Hmm, they were just compiled in, i didn't anything to instrument them with
dtrace.

I've just started a pgbench/dtrace run with instrumented probes aset_alloc,
aset_free and aset_realloc which just counts the calls to them during
pgbench, the first run gives me

tps = 1035.045523 (excluding connections establishing)

Ideas?

--
Thanks

Bernd

Bernd Helmle píše v pá 11. 12. 2009 v 17:13 +0100:
>
> --On 11. Dezember 2009 11:28:54 -0300 Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>
> >>
> >> without compiled probes: AVG(2531.68)
> >> with compiled probes: AVG(2511.97)
> >
> > Were the probes enabled?
>
> Hmm, they were just compiled in, i didn't anything to instrument them with
> dtrace.
>
> I've just started a pgbench/dtrace run with instrumented probes aset_alloc,
> aset_free and aset_realloc which just counts the calls to them during
> pgbench, the first run gives me
>
> tps = 1035.045523 (excluding connections establishing)
>
> Ideas?

When probes are activated they have performance impact. It is normal.
Important is that you can use it when you need it on production system.
No recompilation, no extra binary, no downtime and it is safe.
Performance impact depends on Dscript

Zdenek

On Fri, Dec 11, 2009 at 12:55 PM, Zdenek Kotala <Zdenek(dot)Kotala(at)sun(dot)com> wrote:
> Bernd Helmle píše v pá 11. 12. 2009 v 17:13 +0100:
>>
>> --On 11. Dezember 2009 11:28:54 -0300 Alvaro Herrera
>> <alvherre(at)commandprompt(dot)com> wrote:
>>
>> >>
>> >> without compiled probes: AVG(2531.68)
>> >> with compiled probes: AVG(2511.97)
>> >
>> > Were the probes enabled?
>>
>> Hmm, they were just compiled in, i didn't anything to instrument them with
>> dtrace.
>>
>> I've just started a pgbench/dtrace run with instrumented probes aset_alloc,
>> aset_free and aset_realloc which just counts the calls to them during
>> pgbench, the first run gives me
>>
>> tps = 1035.045523 (excluding connections establishing)
>>
>> Ideas?
>
> When probes are activated they have performance impact. It is normal.
> Important is that you can use it when you need it on production system.
> No recompilation, no extra binary, no downtime and it is safe.
> Performance impact depends on Dscript

Yeah. The problem here is the impact when the probes aren't enabled.

I thought we had an idea of using the AllocSet dispatch mechanism to
make this zero-overhead in the case where the probes are not enabled.
What happened to that notion?

...Robert

Robert Haas píše v pá 11. 12. 2009 v 12:55 -0500:
> On Fri, Dec 11, 2009 at 12:55 PM, Zdenek Kotala <Zdenek(dot)Kotala(at)sun(dot)com> wrote:
> > Bernd Helmle píše v pá 11. 12. 2009 v 17:13 +0100:
> >>
> >> --On 11. Dezember 2009 11:28:54 -0300 Alvaro Herrera
> >> <alvherre(at)commandprompt(dot)com> wrote:
> >>
> >> >>
> >> >> without compiled probes: AVG(2531.68)
> >> >> with compiled probes: AVG(2511.97)
> >> >
> >> > Were the probes enabled?
> >>
> >> Hmm, they were just compiled in, i didn't anything to instrument them with
> >> dtrace.
> >>
> >> I've just started a pgbench/dtrace run with instrumented probes aset_alloc,
> >> aset_free and aset_realloc which just counts the calls to them during
> >> pgbench, the first run gives me
> >>
> >> tps = 1035.045523 (excluding connections establishing)
> >>
> >> Ideas?
> >
> > When probes are activated they have performance impact. It is normal.
> > Important is that you can use it when you need it on production system.
> > No recompilation, no extra binary, no downtime and it is safe.
> > Performance impact depends on Dscript
>
> Yeah. The problem here is the impact when the probes aren't enabled.
>
> I thought we had an idea of using the AllocSet dispatch mechanism to
> make this zero-overhead in the case where the probes are not enabled.
> What happened to that notion?

We know that performance impact is less then 1% probably less then 0.6%.
The question is if it is acceptable or not. I personally think that it
is acceptable. However if not, I will start work on backup solution with
dtraced AllocSet and some switching mechanism. But it needs little
discussion about design. And first we need decision about current
performance impact.

Zdenek

On Fri, Dec 11, 2009 at 1:04 PM, Zdenek Kotala <Zdenek(dot)Kotala(at)sun(dot)com> wrote:
> We know that performance impact is less then 1% probably less then 0.6%.
> The question is if it is acceptable or not. I personally think that it
> is acceptable. However if not, I will start work on backup solution with
> dtraced AllocSet and some switching mechanism. But it needs little
> discussion about design. And first we need decision about current
> performance impact.

As far as I am concerned that is way too much, particularly
considering that your test case isn't designed to be particularly
memory-allocation intensive, and if it is up to me I will reject this.
Even a quarter-percent slowdown for a feature that will be used only
by a small fraction of users only a small fraction of time time seems
totally unacceptable to me.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> As far as I am concerned that is way too much, particularly
> considering that your test case isn't designed to be particularly
> memory-allocation intensive, and if it is up to me I will reject this.
> Even a quarter-percent slowdown for a feature that will be used only
> by a small fraction of users only a small fraction of time time seems
> totally unacceptable to me.

It seems to me that anyone who really needs this can instrument the
alloc functions anyway --- isn't one of the features of DTrace supposed
to be that you can monitor calls to a particular function without any
prearranged code support? Or is that one of the things like "zero
overhead" that turns out to be more marketing-speak than reality?

Anyway I concur with Robert's opinion that the use-case is far too small
to justify incurring a measurable overhead for everybody. There might
be some small argument for putting these in under an extra #ifdef, but
they wouldn't get into any regular production build.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I thought we had an idea of using the AllocSet dispatch mechanism to
> make this zero-overhead in the case where the probes are not enabled.
> What happened to that notion?

I must have missed that discussion, but +1 --- should be possible to get
to zero-overhead-when-off that way. The trick is to figure out
what/where enables the alternate implementation. The current design
assumes that the callers of FooContextCreate choose the implementation,
but we don't want that here.

regards, tom lane

Tom Lane píše v pá 11. 12. 2009 v 13:56 -0500:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > As far as I am concerned that is way too much, particularly
> > considering that your test case isn't designed to be particularly
> > memory-allocation intensive, and if it is up to me I will reject this.
> > Even a quarter-percent slowdown for a feature that will be used only
> > by a small fraction of users only a small fraction of time time seems
> > totally unacceptable to me.
>
> It seems to me that anyone who really needs this can instrument the
> alloc functions anyway --- isn't one of the features of DTrace supposed
> to be that you can monitor calls to a particular function without any
> prearranged code support? Or is that one of the things like "zero
> overhead" that turns out to be more marketing-speak than reality?

There are several types of probes. For example for PID provider probes
you can monitor all entry and return point from global function. And
also you can put probe on each asm instruction in the function. These
probes have zero overhead, because dtrace understand ABI and know where
args are. Unfortunately user defined probes has small overhead which is
price for universal solution which works with all compilers and linkers.

> Anyway I concur with Robert's opinion that the use-case is far too small
> to justify incurring a measurable overhead for everybody.

OK.

> There might
> be some small argument for putting these in under an extra #ifdef, but
> they wouldn't get into any regular production build.

unfortunately #ifdef solution kills main dtrace goal - without
recompilation :(.

Zdenek

Tom Lane píše v pá 11. 12. 2009 v 14:38 -0500:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > I thought we had an idea of using the AllocSet dispatch mechanism to
> > make this zero-overhead in the case where the probes are not enabled.
> > What happened to that notion?
>
> I must have missed that discussion, but +1 --- should be possible to get
> to zero-overhead-when-off that way. The trick is to figure out
> what/where enables the alternate implementation. The current design
> assumes that the callers of FooContextCreate choose the implementation,
> but we don't want that here.

I thought about it. I think we can use GUC variable (e.g. dtraced_alloc)
and hook switch pointers to dtraced AsetFunctions. The problem is how to
distribute to all backend.

Zdenek

Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> I thought about it. I think we can use GUC variable (e.g. dtraced_alloc)
> and hook switch pointers to dtraced AsetFunctions. The problem is how to
> distribute to all backend.

You set the GUC in postgresql.conf. No big deal.

If we go this route it would be nice to think about making a facility
that has some usefulness for non-DTrace platforms too.

regards, tom lane

Tom Lane píše v pá 11. 12. 2009 v 15:11 -0500:
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> > I thought about it. I think we can use GUC variable (e.g. dtraced_alloc)
> > and hook switch pointers to dtraced AsetFunctions. The problem is how to
> > distribute to all backend.
>
> You set the GUC in postgresql.conf. No big deal.
>
> If we go this route it would be nice to think about making a facility
> that has some usefulness for non-DTrace platforms too.

Do you mean general facility for switching memory allocator?

Zdenek

Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> Tom Lane pe v p 11. 12. 2009 v 15:11 -0500:
>> If we go this route it would be nice to think about making a facility
>> that has some usefulness for non-DTrace platforms too.

> Do you mean general facility for switching memory allocator?

No, I was thinking of some sort of memory allocation stats collection
that doesn't depend on DTrace. It's amazing to me that we've never
gone back and improved on the original quick-and-dirty
MemoryContextStats mechanism. I certainly find myself using that a
lot for issues like tracking down memory leaks. While palloc has a lot
of advantages, the fact that you can't easily plug in a debug-friendly
substitute malloc package is not one of them :-(

regards, tom lane

Tom Lane wrote:
> It's amazing to me that we've never
> gone back and improved on the original quick-and-dirty
> MemoryContextStats mechanism. I certainly find myself using that a
> lot for issues like tracking down memory leaks.
That code hasn't really gone anywhere since Neil tweaked the indentation
two years ago. What sorts of improvements were you looking for?

I started on trying to improve this area at one point but didn't get
far. My first step was going to be just wrapping the call into a UDF to
make it easier to reach without having to set loose gdb. I thought that
might expand the possible uses for MemoryContextStats to "help find a
memory leak safely on the production box", and therefore attact more
attention to improving it. People really don't like running gdb on
production, but a UDF dumping the same data wouldn't seem so risky. As
Daniel Farina pointed out to me one day in a "duh" moment, that idea is
quite obviously doomed by the fact that people want dumps from processes
that you won't be talking to in a UDF context. You won't be able to
find a leak in the background writer by dumping the context the UDF can
see. There would need to be some sort of IPC/signaling mechanism in
each process if you wanted it to work everywhere, and once that
realization hit I realized this was a deeper problem than it appeared.

If you want to make it easier to export into user space, it seems to me
the memory context stats data either needs to get pushed somewhere it's
easier to get to (the way the regular stats are), or there needs to be
some way of polling any random PG process to find it--presumably by
passing the request the pid you want to instrument. Neither of which
may really be a reasonable idea at all, particularly since that in most
cases, you're using a debugger to track your leak down only after
reproducing a test case for it.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Fri, Dec 11, 2009 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
>> Tom Lane píše v pá 11. 12. 2009 v 15:11 -0500:
>>> If we go this route it would be nice to think about making a facility
>>> that has some usefulness for non-DTrace platforms too.
>
>> Do you mean general facility for switching memory allocator?
>
> No, I was thinking of some sort of memory allocation stats collection
> that doesn't depend on DTrace.  It's amazing to me that we've never
> gone back and improved on the original quick-and-dirty
> MemoryContextStats mechanism.  I certainly find myself using that a
> lot for issues like tracking down memory leaks.  While palloc has a lot
> of advantages, the fact that you can't easily plug in a debug-friendly
> substitute malloc package is not one of them :-(

This might be nice to have, but I don't think it's necessarily a
prerequisite for a committable patch. For that, I think we just need
something that uses the dispatch mechanism to avoid the overhead in
the normal case.

However, as this is a substantial redesign and there are 4 days left
in the CommitFest, I am going to mark this patch Returned with
Feedback for now.

...Robert

Greg Smith <greg(at)2ndquadrant(dot)com> writes:
> Tom Lane wrote:
>> It's amazing to me that we've never
>> gone back and improved on the original quick-and-dirty
>> MemoryContextStats mechanism.

> That code hasn't really gone anywhere since Neil tweaked the indentation
> two years ago. What sorts of improvements were you looking for?

Not anything like what you were thinking about, apparently ;-). What
I wish for the most often is a way to track palloc usage down to the
particular file/line of the alloc call. This is obviously not something
that we'd pay the overhead for in a standard build, but I would love to
be able to build a debug version that could do it when I needed. There
are malloc substitutes that can do this, but the palloc layer means that
they're not especially useful for debugging the PG backend.

Of course something like that doesn't have to have anything to do with
what Zdenek was wishing for, but it just struck me that creating a
stats-oriented shim layer in the memory context stuff is playing on
pretty much the same turf.

> I started on trying to improve this area at one point but didn't get
> far. My first step was going to be just wrapping the call into a UDF to
> make it easier to reach without having to set loose gdb. I thought that
> might expand the possible uses for MemoryContextStats to "help find a
> memory leak safely on the production box", and therefore attact more
> attention to improving it.

Just for context: the reason MemoryContextStats is designed as it is
is so that it has a reasonable chance of telling you something useful
about an out-of-memory failure. When you're down to your last ten
bytes, it's not going to help if your reporting mechanism wants to
allocate a data structure to put its results in. So it writes to stderr
and nothing else. It may well be that that constraint means we can
never do anything really creative with MemoryContextStats per se.
But I see the use-case for other mechanisms as being to get some info
before we reach the point of having our backs to the wall.

regards, tom lane