Merging scratch/no-purespace to remove unexec and purespace

Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

In August, we decided to remove the unexec build along with purespace.
The scratch/no-purespace branch removes both, and has been rebased on
top of a recent master.

We, the maintainers, believe that the scratch/no-purespace branch is
now ready to merge, and would appreciate any final feedback, testing,
and code reviews.  Specifically, the branch has been primarily tested
on GNU/Linux and macOS, so testing on other systems would be greatly
appreciated.

Unless we uncover any serious blocking issues, or significant or
unexpected objections from the community, we plan to merge the
branch on February 1, 2025.

During our last discussion, we identified some issues with using
pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
Solaris 10 Zone.  As we have explained previously, patches are welcome
for these issues, but we do not currently consider them as blockers
for this merge.

Thanks!

Stefan Kangas, on behalf of the Emacs maintainers

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

> In August, we decided to remove the unexec build along with purespace.
> The scratch/no-purespace branch removes both, and has been rebased on
> top of a recent master.
>
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.
>
> Unless we uncover any serious blocking issues, or significant or
> unexpected objections from the community, we plan to merge the
> branch on February 1, 2025.
>
> During our last discussion, we identified some issues with using
> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
> Solaris 10 Zone.  As we have explained previously, patches are welcome
> for these issues, but we do not currently consider them as blockers
> for this merge.
>
> Thanks!
>
> Stefan Kangas, on behalf of the Emacs maintainers

Building on macOS with --enable-checking=all --with-native-compilation
gives assertion violations. It builds without the --enable-checking.

gmake -C ../lisp compile-first EMACS="../src/bootstrap-emacs"
  ELC+ELN  emacs-lisp/comp-common.elc
  ELC+ELN  emacs-lisp/comp-run.elc
  ELC+ELN  emacs-lisp/radix-tree.elc
  ELC+ELN  emacs-lisp/loaddefs-gen.elc
  ELC+ELN  emacs-lisp/macroexp.elc
  ELC+ELN  emacs-lisp/cconv.elc
  ELC+ELN  emacs-lisp/comp-cstr.elc
  ELC+ELN  emacs-lisp/bytecomp.elc
  ELC+ELN  emacs-lisp/byte-opt.elc
  ELC+ELN  emacs-lisp/comp.elc
gmake -C ../lisp autoloads EMACS="../src/bootstrap-emacs"
  ELC+ELN  ../lisp/cus-face.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
gmake[3]: *** [Makefile:285: ../lisp/cus-face.elc] Abort trap: 6
  ELC+ELN  international/titdic-cnv.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
Fatal error 6: Aborted
gmake[3]: *** [Makefile:330: international/titdic-cnv.elc] Abort trap: 6
gmake[2]: *** [Makefile:901: ../lisp/cus-face.elc] Error 2
gmake[2]: *** Waiting for unfinished jobs....
gmake[2]: *** [Makefile:961: ../lisp/loaddefs.el] Error 2
  ELC+ELN  ../lisp/abbrev.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
gmake[3]: *** [Makefile:285: ../lisp/abbrev.elc] Abort trap: 6
  ELC+ELN  ../lisp/button.elc

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> In August, we decided to remove the unexec build along with purespace.
>> The scratch/no-purespace branch removes both, and has been rebased on
>> top of a recent master.
>>
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>>
>> Unless we uncover any serious blocking issues, or significant or
>> unexpected objections from the community, we plan to merge the
>> branch on February 1, 2025.
>>
>> During our last discussion, we identified some issues with using
>> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
>> Solaris 10 Zone.  As we have explained previously, patches are welcome
>> for these issues, but we do not currently consider them as blockers
>> for this merge.
>>
>> Thanks!
>>
>> Stefan Kangas, on behalf of the Emacs maintainers
>
> Building on macOS with --enable-checking=all --with-native-compilation
> gives assertion violations. It builds without the --enable-checking.
>
> gmake -C ../lisp compile-first EMACS="../src/bootstrap-emacs"
>   ELC+ELN  emacs-lisp/comp-common.elc
>   ELC+ELN  emacs-lisp/comp-run.elc
>   ELC+ELN  emacs-lisp/radix-tree.elc
>   ELC+ELN  emacs-lisp/loaddefs-gen.elc
>   ELC+ELN  emacs-lisp/macroexp.elc
>   ELC+ELN  emacs-lisp/cconv.elc
>   ELC+ELN  emacs-lisp/comp-cstr.elc
>   ELC+ELN  emacs-lisp/bytecomp.elc
>   ELC+ELN  emacs-lisp/byte-opt.elc
>   ELC+ELN  emacs-lisp/comp.elc
> gmake -C ../lisp autoloads EMACS="../src/bootstrap-emacs"
>   ELC+ELN  ../lisp/cus-face.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> gmake[3]: *** [Makefile:285: ../lisp/cus-face.elc] Abort trap: 6
>   ELC+ELN  international/titdic-cnv.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> Fatal error 6: Aborted
> gmake[3]: *** [Makefile:330: international/titdic-cnv.elc] Abort trap: 6
> gmake[2]: *** [Makefile:901: ../lisp/cus-face.elc] Error 2
> gmake[2]: *** Waiting for unfinished jobs....
> gmake[2]: *** [Makefile:961: ../lisp/loaddefs.el] Error 2
>   ELC+ELN  ../lisp/abbrev.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> gmake[3]: *** [Makefile:285: ../lisp/abbrev.elc] Abort trap: 6
>   ELC+ELN  ../lisp/button.elc

I think check_comp_unit_relocs should be removed in the branch. What's
left of it the branch, checks in master if everything has been
put in purespace which should be there. IIUC correctly, of course.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> I think check_comp_unit_relocs should be removed in the branch. What's
> left of it the branch, checks in master if everything has been
> put in purespace which should be there. IIUC correctly, of course.

Patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-check_comp_unit_relocs.patch --]
[-- Type: text/x-patch, Size: 1722 bytes --]

From 66e73f65966e40af67338cfdf18f89549d687935 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerd=20M=C3=B6llmann?= <gerd@gnu.org>
Date: Tue, 17 Dec 2024 15:28:14 +0100
Subject: [PATCH] Remove check_comp_unit_relocs

* src/comp.c (check_comp_unit_relocs): Removed.
(load_comp_unit): Remove use.
---
 src/comp.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index f91f7c57f45..2c7f2b704f6 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -5164,32 +5164,6 @@ load_static_obj (struct Lisp_Native_Comp_Unit *comp_u, const char *name)
 
 }
 
-/* Return false when something is wrong or true otherwise.  */
-
-static bool
-check_comp_unit_relocs (struct Lisp_Native_Comp_Unit *comp_u)
-{
-  dynlib_handle_ptr handle = comp_u->handle;
-  Lisp_Object *data_relocs = dynlib_sym (handle, DATA_RELOC_SYM);
-
-  EMACS_INT d_vec_len = XFIXNUM (Flength (comp_u->data_vec));
-
-  for (ptrdiff_t i = 0; i < d_vec_len; i++)
-    {
-      Lisp_Object x = data_relocs[i];
-      if (EQ (x, Qlambda_fixup))
-	return false;
-      else if (NATIVE_COMP_FUNCTIONP (x))
-	{
-	  if (NILP (Fgethash (x, comp_u->lambda_gc_guard_h, Qnil)))
-	    return false;
-	}
-      else if (!EQ (x, AREF (comp_u->data_vec, i)))
-	return false;
-    }
-  return true;
-}
-
 static void
 unset_cu_load_ongoing (Lisp_Object comp_u)
 {
@@ -5319,7 +5293,6 @@ load_comp_unit (struct Lisp_Native_Comp_Unit *comp_u, bool loading_dump,
       /* Make sure data_ephemeral_vec still exists after top_level_run has run.
 	 Guard against sibling call optimization (or any other).  */
       data_ephemeral_vec = data_ephemeral_vec;
-      eassert (check_comp_unit_relocs (comp_u));
     }
 
   if (!recursive_load)
-- 
2.47.1

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> I think check_comp_unit_relocs should be removed in the branch. What's
>> left of it the branch, checks in master if everything has been
>> put in purespace which should be there. IIUC correctly, of course.
>
> Patch attached.

Pushed. Complaints to me please.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>  <monnier@iro.umontreal.ca>
> Date: Tue, 17 Dec 2024 18:56:05 +0100
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> >> I think check_comp_unit_relocs should be removed in the branch. What's
> >> left of it the branch, checks in master if everything has been
> >> put in purespace which should be there. IIUC correctly, of course.
> >
> > Patch attached.
> 
> Pushed. Complaints to me please.

I'd like at least Andrea to take a look and confirm.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>  <monnier@iro.umontreal.ca>
>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>> 
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> 
>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> >
>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>> >> left of it the branch, checks in master if everything has been
>> >> put in purespace which should be there. IIUC correctly, of course.
>> >
>> > Patch attached.
>> 
>> Pushed. Complaints to me please.
>
> I'd like at least Andrea to take a look and confirm.

It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Kangas wrote:

[...]
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.

Do you have an estimate what removing purespace will cost in terms of GC
time?  I mean something like "1ms per collection".  Or perhaps a
suggestion how I could measure it?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Monnier]

>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>
> Do you have an estimate what removing purespace will cost in terms of GC
> time?  I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

In the pdump case it should have no effect at all, or maybe even
a slight *speedup*.

That's because the pdump already fails to take advantage of the
purespace (i.e. the GC traces through the purespace like the rest of
the heap).


        Stefan

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> I think check_comp_unit_relocs should be removed in the branch. What's
>>> left of it the branch, checks in master if everything has been
>>> put in purespace which should be there. IIUC correctly, of course.
>>
>> Patch attached.
>
> Pushed. Complaints to me please.

Thanks!

Re: Merging scratch/no-purespace to remove unexec and purespace

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> In August, we decided to remove the unexec build along with purespace.
> The scratch/no-purespace branch removes both, and has been rebased on
> top of a recent master.
>
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.
>
> Unless we uncover any serious blocking issues, or significant or
> unexpected objections from the community, we plan to merge the
> branch on February 1, 2025.
>
> During our last discussion, we identified some issues with using
> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
> Solaris 10 Zone.  As we have explained previously, patches are welcome
> for these issues, but we do not currently consider them as blockers
> for this merge.

Please don't merge this until Emacs 30 is released, whether that be
before the 1st of February or after.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> In August, we decided to remove the unexec build along with purespace.
>> The scratch/no-purespace branch removes both, and has been rebased on
>> top of a recent master.
>>
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>>
>> Unless we uncover any serious blocking issues, or significant or
>> unexpected objections from the community, we plan to merge the
>> branch on February 1, 2025.
>>
>> During our last discussion, we identified some issues with using
>> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
>> Solaris 10 Zone.  As we have explained previously, patches are welcome
>> for these issues, but we do not currently consider them as blockers
>> for this merge.
>
> Please don't merge this until Emacs 30 is released, whether that be
> before the 1st of February or after.

I'm guessing here, but I take that to mean:
1. You intend to work on resolving the problems with pdumper on one or
   more legacy proprietary systems.
2. You will not have much time before Emacs 30 is out, as you have
   previously indicated.

Is that a correct understanding?  If not, may I ask that you elaborate?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> We, the maintainers, believe that the scratch/no-purespace branch is
>>> now ready to merge, and would appreciate any final feedback, testing,
>>> and code reviews.  Specifically, the branch has been primarily tested
>>> on GNU/Linux and macOS, so testing on other systems would be greatly
>>> appreciated.
>>
>> Do you have an estimate what removing purespace will cost in terms of GC
>> time?  I mean something like "1ms per collection".  Or perhaps a
>> suggestion how I could measure it?
>
> In the pdump case it should have no effect at all, or maybe even
> a slight *speedup*.
>
> That's because the pdump already fails to take advantage of the
> purespace (i.e. the GC traces through the purespace like the rest of
> the heap).

I'll note that the best solution to that is to have a generational GC
instead.  Simple, right?

It's not entirely unrelated though: among other things, one reason why
merging this would be good is that it would reportedly simplify the work
on the igc branch.  And indeed any GC-related work now or in future.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Monnier wrote:

>> Do you have an estimate what removing purespace will cost in terms of GC
>> time?  I mean something like "1ms per collection".  Or perhaps a
>> suggestion how I could measure it?
>
> In the pdump case it should have no effect at all, or maybe even
> a slight *speedup*.
>
> That's because the pdump already fails to take advantage of the
> purespace (i.e. the GC traces through the purespace like the rest of
> the heap).

So the pdumper copies objects from purespace to the dump like normal
objects; when loading the dump, purespace stays empty.

I had (wrongly) assumed that the pdumper creates a separate section for
pure objects.  Creating such a section sounds easy enough (hmm, maybe
not so easy because of hashtables).  Still not sure if it would be worth
the effort.

Helmut

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Kangas wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
[...]
>> That's because the pdump already fails to take advantage of the
>> purespace (i.e. the GC traces through the purespace like the rest of
>> the heap).
>
> I'll note that the best solution to that is to have a generational GC
> instead.  Simple, right?

A generational GC is definitely simpler.  Whether it's the "best"
solution is not so clear: A copying GC, like MPS, still needs to trace
and copy pure objects whenever the oldest generation is in the condemned
set.  Moving pure objects to a non-moving pool might be better.

> It's not entirely unrelated though: among other things, one reason why
> merging this would be good is that it would reportedly simplify the work
> on the igc branch.  And indeed any GC-related work now or in future.

Objects in purespace are immutable and immortal.  That's potentially
useful information for the GC.  Removing purespace also removes that
information.  Of course, if the pdumper already throws away this
information, then purespace just adds useless complexity.

Helmut

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

"Helmut Eller" <eller.helmut@gmail.com> writes:

> On Tue, Dec 17 2024, Stefan Kangas wrote:
>
> [...]
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>
> Do you have an estimate what removing purespace will cost in terms of GC
> time?  I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

"Close to zero" is the best I can do at this time.  In particular, that
implies "not catastrophically worse", which is all that is relevant
right now, IMHO.

> I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

I think that all the previously-pure data should be available right away
after starting an Emacs session, so maybe something like:

perf record -e cycles ./src/emacs -Q --batch --eval '(dotimes (i 1000)
(garbage-collect)))'

would be one data point.  Some effects would only be visible in GCs with
deep call stacks, or in large sessions, but that's hard to measure.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Andrea Corallo]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>>  <monnier@iro.umontreal.ca>
>>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>>> 
>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>> 
>>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>> >
>>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>>> >> left of it the branch, checks in master if everything has been
>>> >> put in purespace which should be there. IIUC correctly, of course.
>>> >
>>> > Patch attached.
>>> 
>>> Pushed. Complaints to me please.
>>
>> I'd like at least Andrea to take a look and confirm.
>
> It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c

Hi Gerd,

looking at the commit now, why do you think 'check_comp_unit_relocs'
should be removed?  

Even if now the situation is simpler 'check_comp_unit_relocs' is still
performing some sanity checks like if lambdas are all been fixed-up and
present in 'comp_u->lambda_gc_guard_h'.

  Andrea