last_marked array is now ifdef'ed away

last_marked array is now ifdef'ed away

[Eli Zaretskii]

Commit 7a8798de95a5 (from Apr 4 2022) made the last_marked[] array and
the related machinery of tracing the marked objects #ifdef'ed away by
default:

  +/* Whether to remember a few of the last marked values for debugging.  */
  +#define GC_REMEMBER_LAST_MARKED 0
  +
  +#if GC_REMEMBER_LAST_MARKED
   enum { LAST_MARKED_SIZE = 1 << 9 }; /* Must be a power of 2.  */
   Lisp_Object last_marked[LAST_MARKED_SIZE] EXTERNALLY_VISIBLE;
   static int last_marked_index;
  +#endif
  [...]
  -  last_marked[last_marked_index++] = obj;
  -  last_marked_index &= LAST_MARKED_SIZE - 1;
  +#if GC_REMEMBER_LAST_MARKED
  +      last_marked[last_marked_index++] = obj;
  +      last_marked_index &= LAST_MARKED_SIZE - 1;
  +#endif

I don't remember this aspect being discussed, and the commit log
message doesn't even mention the change, let alone provides a
rationale for it.

Mattias, why was this done?  Are the changes you introduced in that
changeset somehow incompatible with the last_marked[] facility?  If
not, I think we should make this again compiled-in by default, because
IME it is a valuable means of debugging GC problems.

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

Ping!  Mattias, could you please respond?

> Date: Sat, 07 Sep 2024 09:05:07 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Commit 7a8798de95a5 (from Apr 4 2022) made the last_marked[] array and
> the related machinery of tracing the marked objects #ifdef'ed away by
> default:
> 
>   +/* Whether to remember a few of the last marked values for debugging.  */
>   +#define GC_REMEMBER_LAST_MARKED 0
>   +
>   +#if GC_REMEMBER_LAST_MARKED
>    enum { LAST_MARKED_SIZE = 1 << 9 }; /* Must be a power of 2.  */
>    Lisp_Object last_marked[LAST_MARKED_SIZE] EXTERNALLY_VISIBLE;
>    static int last_marked_index;
>   +#endif
>   [...]
>   -  last_marked[last_marked_index++] = obj;
>   -  last_marked_index &= LAST_MARKED_SIZE - 1;
>   +#if GC_REMEMBER_LAST_MARKED
>   +      last_marked[last_marked_index++] = obj;
>   +      last_marked_index &= LAST_MARKED_SIZE - 1;
>   +#endif
> 
> I don't remember this aspect being discussed, and the commit log
> message doesn't even mention the change, let alone provides a
> rationale for it.
> 
> Mattias, why was this done?  Are the changes you introduced in that
> changeset somehow incompatible with the last_marked[] facility?  If
> not, I think we should make this again compiled-in by default, because
> IME it is a valuable means of debugging GC problems.
> 
> 

Re: last_marked array is now ifdef'ed away

[Mattias Engdegård]

12 sep. 2024 kl. 11.19 skrev Eli Zaretskii <eliz@gnu.org>:

>> Commit 7a8798de95a5 (from Apr 4 2022) made the last_marked[] array and
>> the related machinery of tracing the marked objects #ifdef'ed away by
>> default:
>> 
>>  +/* Whether to remember a few of the last marked values for debugging.  */
>>  +#define GC_REMEMBER_LAST_MARKED 0

Yes, debugging code in the inner loop of one of the hottest paths of the GC probably shouldn't be on by default. I've done a large amount of intricate GC debugging and never felt a need for it, but it is easy enough to enable when wanted.

We could hitch it to another existing config option like ENABLE_CHECKING, or even make this an independent config option, but there's value in not having to reconfigure and recompile everything when turning it on so keeping it localised in the source seems the better choice.

We could also change the `#if` to `if` to reduce the risk of bitrot. I'll do that later.

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Fri, 13 Sep 2024 16:43:49 +0200
> Cc: emacs-devel@gnu.org
> 
> 12 sep. 2024 kl. 11.19 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >> Commit 7a8798de95a5 (from Apr 4 2022) made the last_marked[] array and
> >> the related machinery of tracing the marked objects #ifdef'ed away by
> >> default:
> >> 
> >>  +/* Whether to remember a few of the last marked values for debugging.  */
> >>  +#define GC_REMEMBER_LAST_MARKED 0
> 
> Yes, debugging code in the inner loop of one of the hottest paths of the GC probably shouldn't be on by default. I've done a large amount of intricate GC debugging and never felt a need for it, but it is easy enough to enable when wanted.

But if a user reports a crash inside GC, that's basically the only
trace we have to try and understand what caused the crash, especially
if the debug info was stripped.  That's why this code was always on by
default.

> We could hitch it to another existing config option like ENABLE_CHECKING, or even make this an independent config option, but there's value in not having to reconfigure and recompile everything when turning it on so keeping it localised in the source seems the better choice.
> 
> We could also change the `#if` to `if` to reduce the risk of bitrot. I'll do that later.

My main concern is not bitrot, it's actually being able to debug
crashes inside GC.

I think we should turn it on again, and remove the conditions.

Re: last_marked array is now ifdef'ed away

[Mattias Engdegård]

13 sep. 2024 kl. 17.21 skrev Eli Zaretskii <eliz@gnu.org>:

> But if a user reports a crash inside GC, that's basically the only
> trace we have to try and understand what caused the crash, especially
> if the debug info was stripped.  That's why this code was always on by
> default.

Yes, I understand that -- nevertheless it is a bit of a waste to spend time on something that is only rarely used (apparently it took over 2 years before anyone noticed, and I know you aren't one for keeping quiet about this sort of thing).

It surprised me how expensive this silly thing is: enabling GC_REMEMBER_LAST_MARKED made GC 4.5-6.5 % slower in some simple benchmarks, and the difference showed up in some actual useful work type of code that I measured.

And that is with LAST_MARKED_SIZE downgraded from the default 512 to just 16 which seemed more reasonable. (512 entries is 4 KiB of L1D cache which seems extravagant, but I presume the size wasn't given much thought at the time.)

Perhaps it's not a big surprise after all: the GC marking loop has data-dependent branches and data-dependent loads, and we are likely to be bound by instructions in flight rather than execution resources; maybe any added instructions and memory traffic will just make it worse.

> My main concern is not bitrot, it's actually being able to debug
> crashes inside GC.

I'm quite sympathetic to that goal but it would be good if we could do it more cheaply. You can inspect the mark_stack structure; I've found this to help quite a bit.

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sat, 14 Sep 2024 19:05:31 +0200
> Cc: emacs-devel@gnu.org
> 
> 13 sep. 2024 kl. 17.21 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > But if a user reports a crash inside GC, that's basically the only
> > trace we have to try and understand what caused the crash, especially
> > if the debug info was stripped.  That's why this code was always on by
> > default.
> 
> Yes, I understand that -- nevertheless it is a bit of a waste to spend time on something that is only rarely used (apparently it took over 2 years before anyone noticed, and I know you aren't one for keeping quiet about this sort of thing).

That's the nature of things that are needed rarely.  But when they
_are_ needed, there's nothing to replace them, and that's when you
appreciate their existence.  Like safety belts, these features are
never a waste.

> And that is with LAST_MARKED_SIZE downgraded from the default 512 to just 16 which seemed more reasonable. (512 entries is 4 KiB of L1D cache which seems extravagant, but I presume the size wasn't given much thought at the time.)

16 is way too few.  GC is deeply recursive, and when I used this, I
almost always had to look at much more than 16 objects.

> Perhaps it's not a big surprise after all: the GC marking loop has data-dependent branches and data-dependent loads, and we are likely to be bound by instructions in flight rather than execution resources; maybe any added instructions and memory traffic will just make it worse.
> 
> > My main concern is not bitrot, it's actually being able to debug
> > crashes inside GC.
> 
> I'm quite sympathetic to that goal but it would be good if we could do it more cheaply. You can inspect the mark_stack structure; I've found this to help quite a bit.

That's not the same, and doesn't show all the objects.

I'm quite confident that I want this be turned on again.

Re: last_marked array is now ifdef'ed away

[Mattias Engdegård]

14 sep. 2024 kl. 19.30 skrev Eli Zaretskii <eliz@gnu.org>:

> That's the nature of things that are needed rarely.  But when they
> _are_ needed, there's nothing to replace them, and that's when you
> appreciate their existence.  Like safety belts, these features are
> never a waste.

No, this device doesn't improve safety in any way. At best it's a special-purpose single-channel flight data recorder mounted in such a way that it disrupts the airflow and increases fuel consumption. Let's see if we can make it a bit more streamlined.

> 16 is way too few.  GC is deeply recursive, and when I used this, I
> almost always had to look at much more than 16 objects.

Note that the GC recursion is now almost entirely handled by mark_stack. 512 entries is an entire x86 page; surely that can be trimmed?

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sat, 14 Sep 2024 22:30:11 +0200
> Cc: emacs-devel@gnu.org
> 
> 14 sep. 2024 kl. 19.30 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > That's the nature of things that are needed rarely.  But when they
> > _are_ needed, there's nothing to replace them, and that's when you
> > appreciate their existence.  Like safety belts, these features are
> > never a waste.
> 
> No, this device doesn't improve safety in any way. At best it's a special-purpose single-channel flight data recorder mounted in such a way that it disrupts the airflow and increases fuel consumption. Let's see if we can make it a bit more streamlined.

I'm okay with FDR analogy.  As anyone who saw Air Crash Investigation
series knows, this is invaluable in investigating the (very rare!)
airplane crashes.

> > 16 is way too few.  GC is deeply recursive, and when I used this, I
> > almost always had to look at much more than 16 objects.
> 
> Note that the GC recursion is now almost entirely handled by mark_stack. 512 entries is an entire x86 page; surely that can be trimmed?

I'd need to see actual evidence for a shorter array, based on actual
GC run of some data structure.

Also, how bad is the penalty for accessing 512-element array vs a
16-element array?  Unless it's close to 10% or exceeds that, I
wouldn't bother.

Re: last_marked array is now ifdef'ed away

[Mattias Engdegård]

15 sep. 2024 kl. 07.51 skrev Eli Zaretskii <eliz@gnu.org>:

> I'd need to see actual evidence for a shorter array, based on actual
> GC run of some data structure.

And I'd need to see actual evidence for a longer array, based on actual debugging cases.

But perhaps there is a way for both of us to be just slightly disappointed: what about gating the check with a flag? That way, the trace buffer is always compiled-in. It needs to be enabled by the user but that is straightforward and can be done on the command line, in Lisp, or in a debugger.

As expected the runtime cost is not quite as bad as keeping it on all the time – the branch is always correctly predicted – although still slower than I'd like, but it's the best I can do for now.

There is then no need to skimp on the trace buffer size since it only matters when actually enabled. (We could even make it dynamic, so you'd set it to a megabyte if the situation calls for that.)

And I have a patch ready.

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 15 Sep 2024 15:58:03 +0200
> Cc: emacs-devel@gnu.org
> 
> 15 sep. 2024 kl. 07.51 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I'd need to see actual evidence for a shorter array, based on actual
> > GC run of some data structure.
> 
> And I'd need to see actual evidence for a longer array, based on actual debugging cases.

There's no need for any evidence to keep the code which we always had.
Evidence is needed when changes are suggested that modify stuff in
significant ways.

> But perhaps there is a way for both of us to be just slightly disappointed: what about gating the check with a flag? That way, the trace buffer is always compiled-in. It needs to be enabled by the user but that is straightforward and can be done on the command line, in Lisp, or in a debugger.

That won't help because GC crashes are seldom if ever reproducible.
So if the trace is off, the information is gone and cannot be
recovered in practice.

I'm not just being stubborn, there really is no way around this.
Leaving this disabled, no matter in what way, basically means we
delete the feature.  Because it can never be enabled in time for the
problem to strike.

Re: last_marked array is now ifdef'ed away

[Mattias Engdegård]

15 sep. 2024 kl. 16.21 skrev Eli Zaretskii <eliz@gnu.org>:

> There's no need for any evidence to keep the code which we always had.

Well, we didn't have it in Emacs 29, so one might be excused for thinking it wasn't a strict necessity.

> That won't help because GC crashes are seldom if ever reproducible.
> So if the trace is off, the information is gone and cannot be
> recovered in practice.

My experience is rather that in such cases the crash was already gone because there was no core dump or debugger attached anyway, but we then take the necessary steps to catch the bug next time – turn on core dumps (if possible), run with a debugger, enable checking, etc – and we always end up trapping the gremlin eventually.

Anyway, I'm going to re-enable the mark trace buffer for the sake of development peace; since it's important to you, that's also worth something. I shall add a configuration option for disabling it, with its trade-off clearly documented, so that users can make an informed decision, but the buffer will be enabled by default.


16 sep. 2024 kl. 20.07 skrev Andrea Corallo <acorallo@gnu.org>:

> 5% slowdown during GC is at worst 2.5% slowdown overall for GC intensive
> workloads.

It may not sound much but to a performance engineer this is actually quite a catch. This is because it's additive and independent of other improvements, and Emacs GC being what it is, it's also a matter of latency. If we only bothered with performance changes that give 50 % speed-up or more then we would never get anywhere at all.

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Tue, 17 Sep 2024 12:15:16 +0200
> Cc: emacs-devel@gnu.org,
>  Andrea Corallo <acorallo@gnu.org>
> 
> 15 sep. 2024 kl. 16.21 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > There's no need for any evidence to keep the code which we always had.
> 
> Well, we didn't have it in Emacs 29, so one might be excused for thinking it wasn't a strict necessity.

Because it was silently disabled.  This change should have been
discussed in advance.

> > That won't help because GC crashes are seldom if ever reproducible.
> > So if the trace is off, the information is gone and cannot be
> > recovered in practice.
> 
> My experience is rather that in such cases the crash was already gone because there was no core dump or debugger attached anyway, but we then take the necessary steps to catch the bug next time – turn on core dumps (if possible), run with a debugger, enable checking, etc – and we always end up trapping the gremlin eventually.

At least some (many?) users have core dumps available, even though
current systems foolishly disable them by default.  I see this all the
time on debbugs: users report bugs and tell they have core files or
the crashed Emacs in a debugger.

> Anyway, I'm going to re-enable the mark trace buffer for the sake of development peace; since it's important to you, that's also worth something. I shall add a configuration option for disabling it, with its trade-off clearly documented, so that users can make an informed decision, but the buffer will be enabled by default.

Thank you.  Having a knob to disable it is fine, as long as the
default is to have it enabled.

Re: last_marked array is now ifdef'ed away

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm quite confident that I want this be turned on again.

Since this affects performance by quite a bit (4.5-6.5 %, reportedly),
how about the suggestion to "hitch it to another existing config option
like ENABLE_CHECKING, or even make this an independent config option"?

Re: last_marked array is now ifdef'ed away

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 14 Sep 2024 15:50:03 -0700
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm quite confident that I want this be turned on again.
> 
> Since this affects performance by quite a bit (4.5-6.5 %, reportedly),
> how about the suggestion to "hitch it to another existing config option
> like ENABLE_CHECKING, or even make this an independent config option"?

This feature is useless if it is optional, because its main (perhaps
the only) use is when Emacs crashes during GC on a user machine,
especially without enough debug info.  Crashes during GC are rarely if
at all reproducible, so the only hope to investigate them is to look
at the objects recorded in the last_marked array, and asking users to
rebuild with some non-default option will not help.

If we are going to make it a compilation option, we might as well
remove it entirely.

5% of slowdown during GC (if that's a well-established number) is a
small price to pay for the possibility of investigating rare crashes.
A typical GC cycle takes tens or hundreds of milliseconds, so these
few percents are just a couple of msec.

Re: last_marked array is now ifdef'ed away

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Sat, 14 Sep 2024 15:50:03 -0700
>> Cc: emacs-devel@gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'm quite confident that I want this be turned on again.
>> 
>> Since this affects performance by quite a bit (4.5-6.5 %, reportedly),
>> how about the suggestion to "hitch it to another existing config option
>> like ENABLE_CHECKING, or even make this an independent config option"?
>
> This feature is useless if it is optional, because its main (perhaps
> the only) use is when Emacs crashes during GC on a user machine,
> especially without enough debug info.  Crashes during GC are rarely if
> at all reproducible, so the only hope to investigate them is to look
> at the objects recorded in the last_marked array, and asking users to
> rebuild with some non-default option will not help.
>
> If we are going to make it a compilation option, we might as well
> remove it entirely.
>
> 5% of slowdown during GC (if that's a well-established number) is a
> small price to pay for the possibility of investigating rare crashes.
> A typical GC cycle takes tens or hundreds of milliseconds, so these
> few percents are just a couple of msec.

5% slowdown during GC is at worst 2.5% slowdown overall for GC intensive
workloads.

I think as well we should not loose this debug tool for a slowdown that
no-one would notice.  So I'm for re-enabling the code without it being
under a default-off flag.

  Andrea