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

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:
>
>> 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.

I think we should rephrase that without presupposing the existence of
"pure" objects: Hinting to MPS that an object is expected to be
immutable and have a very long lifetime may have advantages, and that's
a potential reason for introducing (and maintaining) a hinting
mechanism.

On the other hand, it's very common for objects to have those properties
without us knowing in advance that they will, so it's important MPS
works well in the absence of such hints.

>> 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.

I may be misunderstanding what you mean by "immutable", but the most
important property "pure" objects had was that they only referenced
other pure objects or static objects, so GC didn't need to look at them
(but, IIUC, this optimization never worked in pdumper builds).  This
required us to make such objects read-only (which caused problems) and
immortal.

Immortality was an undesirable, but necessary, side effect of the "pure"
optimization, not a feature.  IMHO, so was immutability, but some people
consider it a feature not to be able to modify certain objects.

For example, on the no-purespace branch, you can execute

(aset (symbol-name nil) 0 ?N)

and rename nil to Nil, which will make the rest of your Emacs session
unusable.

On master, this code should (and did at one point, IIRC) throw a
CHECK_IMPURE exception.  Right now, it segfaults, which demonstrates
that the current purespace code has suffered from some bit rot and
removing purespace will fix some bugs for that reason alone.

So the two major features of pure space are broken right now.  Fixing
that is an option, and it's the only way we'd ever see a fair comparison
of purespace and no-purespace performance, but I hope it's not going to
happen.

So what's left is a weak hint to the GC that this object is likely to
remain reachable, and not to be modified, but for MPS to set up a "pure"
space of such objects based on this hint seems to me to be an
unrealistic expectation.

> Removing purespace also removes that information.

Removing purespace does leave us with very few read-only objects.  We
can consider introducing a useful set of read-only objects (and agree on
what that even means) after purespace is removed.

However, I would be surprised if the lost information about whether an
object used to be pure turns out to be very useful.

> Of course, if the pdumper already throws away this
> information, then purespace just adds useless complexity.

I believe that's the case.  In fact, once we remove purespace, we can
look at improving how pdumper dumps are handled during GC.  Maybe we can
make it so the pdumper dump looks even more like an ordinary MPS segment
in memory, and then we don't need to treat pdumper objects specially.

Pip

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

[Gerd Möllmann]

Andrea Corallo <acorallo@gnu.org> writes:

> 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

Hi Andrea.

The check you mention checks something that I don't see how it could
happen. (With the usual disclaimers, because it's been some time since I
was in that code for igc.)

When we comp--register-lambda the code putting the result of make_subr
into the hash table and putting it in data_relocs (in the branch) is
just a handful of lines apart. I thereforeo thought we could do without.

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

[Pip Cet via Emacs development discussions.]

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

> Andrea Corallo <acorallo@gnu.org> writes:
>
>> 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
>
> Hi Andrea.
>
> The check you mention checks something that I don't see how it could
> happen. (With the usual disclaimers, because it's been some time since I
> was in that code for igc.)

I'm confused, I thought you did hit the assertion?

AFAICT, the problem is simply that comp.el uses the symbol lambda-fixup.

So a relocation for that symbol is emitted.  But no fixup is, because
this isn't an actual lambda, it's merely the symbol.  The debug code
then sees "lambda-fixup", assumes it's a failed fixup, and asserts.

IOW, the old code happened not to run into this problem because
lambda-fixup was pure, and we never applied the sanity checks to the
pure section.

This "fix" appears to work:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index dbd14b2740d..5d364e77e66 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3254,7 +3254,7 @@ comp--finalize-container
                  ;; from the corresponding m-var.
                  collect (if (gethash obj
                                       (comp-ctxt-byte-func-to-func-h comp-ctxt))
-                             'lambda-fixup
+                             (intern (concat "lambda" (make-string 1 ?-) "fixup"))
                            obj))))
 
 (defun comp--finalize-relocs ()

My suggestion is to fix the "sanity check" on the master branch, change
it to apply to pure relocs there, and restore the fixed check on
scratch/no-purespace afterwards.

Pip

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

[Pip Cet via Emacs development discussions.]

Pip Cet <pipcet@protonmail.com> writes:
> IOW, the old code happened not to run into this problem because
> lambda-fixup was pure, and we never applied the sanity checks to the
> pure section.

Just to be clear: the code on master is fine.  I misunderstood it when
modifying it for purespace removal, resulting in my bug which Gerd
discovered and fixed.  The code on no-purespace is also fine now, but
it's Andrea's call whether he wants some of the checking code restored,
and how.

> My suggestion is to fix the "sanity check" on the master branch, change
> it to apply to pure relocs there, and restore the fixed check on
> scratch/no-purespace afterwards.

Please ignore that. My suggestion is to EXTEND the sanity check on the
master branch to cover pure and impure relocs, and restore the EXTENDED
check before merging scratch/no-purespace.

There is no bug to fix on master.

Pip

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

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Andrea Corallo <acorallo@gnu.org> writes:
>>
>>> 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
>>
>> Hi Andrea.
>>
>> The check you mention checks something that I don't see how it could
>> happen. (With the usual disclaimers, because it's been some time since I
>> was in that code for igc.)
>
> I'm confused, I thought you did hit the assertion?

Correct, building with --enable-checking failed. What confuses?