Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> branch: scratch/igc
> commit 7057c3562ff373c6061b2b9e6d124fb5f6b90ee5
> Author: Jeff Walsh <fejfighter@gmail.com>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.
> ---
>  src/pgtkterm.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/pgtkterm.c b/src/pgtkterm.c
> index 49b7ea406f8..7279f5d863b 100644
> --- a/src/pgtkterm.c
> +++ b/src/pgtkterm.c
> @@ -336,6 +336,7 @@ evq_flush (struct input_event *hold_quit)
>    return n;
>  }
>  
> +#ifndef HAVE_MPS
>  void
>  mark_pgtkterm (void)
>  {
> @@ -369,6 +370,7 @@ mark_pgtkterm (void)
>  	mark_object (device->name);
>      }
>  }
> +#endif
>  
>  char *
>  get_keysym_name (int keysym)

Doesn't this require that device structures be registered and
deregistered as GC roots when allocated and deallocated?  The same
applies to objects in the module's private event queue.

Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

[Fejfighter]

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

Yeah that makes sense and is likely the cause of the crashes I have on that
branch, rather than other common code.

I can't promise I'll have something up soon, but I will try.



On Thu, Jun 27, 2024 at 4:18 PM Po Lu <luangruo@yahoo.com> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
> > branch: scratch/igc
> > commit 7057c3562ff373c6061b2b9e6d124fb5f6b90ee5
> > Author: Jeff Walsh <fejfighter@gmail.com>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> >
> >     * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.
> > ---
> >  src/pgtkterm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/src/pgtkterm.c b/src/pgtkterm.c
> > index 49b7ea406f8..7279f5d863b 100644
> > --- a/src/pgtkterm.c
> > +++ b/src/pgtkterm.c
> > @@ -336,6 +336,7 @@ evq_flush (struct input_event *hold_quit)
> >    return n;
> >  }
> >
> > +#ifndef HAVE_MPS
> >  void
> >  mark_pgtkterm (void)
> >  {
> > @@ -369,6 +370,7 @@ mark_pgtkterm (void)
> >       mark_object (device->name);
> >      }
> >  }
> > +#endif
> >
> >  char *
> >  get_keysym_name (int keysym)
>
> Doesn't this require that device structures be registered and
> deregistered as GC roots when allocated and deallocated?  The same
> applies to objects in the module's private event queue.
>

[-- Attachment #2: Type: text/html, Size: 2013 bytes --]

Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

[Fejfighter]

[-- Attachment #1.1: Type: text/plain, Size: 1968 bytes --]

Comparing to xterm.c and scanning other parts of the pgtk specific code I
only found this as the only allocation unique to PGTK as much of the things
xterm needs to worry about is managed by gtk.

Could someone apply this patch to the scratch/igc branch please?

I also ran the benchmark suite with the latest changes and it's the first
time that it has not crashed emacs/oom'd out or otherwise destabilized my
machine,
It is starting to appear to be more stable for my use cases

Thanks for the hard work.
Jeff Walsh

On Thu, Jun 27, 2024 at 4:51 PM Fejfighter <fejfighter@gmail.com> wrote:

> Yeah that makes sense and is likely the cause of the crashes I have on
> that branch, rather than other common code.
>
> I can't promise I'll have something up soon, but I will try.
>
>
>
> On Thu, Jun 27, 2024 at 4:18 PM Po Lu <luangruo@yahoo.com> wrote:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > branch: scratch/igc
>> > commit 7057c3562ff373c6061b2b9e6d124fb5f6b90ee5
>> > Author: Jeff Walsh <fejfighter@gmail.com>
>> > Commit: Eli Zaretskii <eliz@gnu.org>
>> >
>> >     * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.
>> > ---
>> >  src/pgtkterm.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/src/pgtkterm.c b/src/pgtkterm.c
>> > index 49b7ea406f8..7279f5d863b 100644
>> > --- a/src/pgtkterm.c
>> > +++ b/src/pgtkterm.c
>> > @@ -336,6 +336,7 @@ evq_flush (struct input_event *hold_quit)
>> >    return n;
>> >  }
>> >
>> > +#ifndef HAVE_MPS
>> >  void
>> >  mark_pgtkterm (void)
>> >  {
>> > @@ -369,6 +370,7 @@ mark_pgtkterm (void)
>> >       mark_object (device->name);
>> >      }
>> >  }
>> > +#endif
>> >
>> >  char *
>> >  get_keysym_name (int keysym)
>>
>> Doesn't this require that device structures be registered and
>> deregistered as GC roots when allocated and deallocated?  The same
>> applies to objects in the module's private event queue.
>>
>

[-- Attachment #1.2: Type: text/html, Size: 3041 bytes --]

[-- Attachment #2: 0001-src-pgtkterm.c-pgtk_term_init-allocated-dpyinfo-as-a.patch --]
[-- Type: application/x-patch, Size: 1184 bytes --]

Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

[Gerd Möllmann]

Fejfighter <fejfighter@gmail.com> writes:

> Could someone apply this patch to the scratch/igc branch please?

Thanks Jeff, I've pushed it. 

Re: scratch/igc 7057c3562ff: * src/pgtkterm.c (mark_pgtkterm): Exclude from MPS builds.

[Pip Cet]

On Tuesday, July 23rd, 2024 at 05:12, Fejfighter <fejfighter@gmail.com> wrote:
> Comparing to xterm.c and scanning other parts of the pgtk specific code I only found this as the only allocation unique to PGTK as much of the things xterm needs to worry about is managed by gtk.

Thanks!

> Could someone apply this patch to the scratch/igc branch please?

Will do.

> I also ran the benchmark suite with the latest changes and it's the first time that it has not crashed emacs/oom'd out or otherwise destabilized my machine,
> It is starting to appear to be more stable for my use cases

That's great to hear! Running the benchmark suite should, of course, work. However, performance can be improved a lot by playing with the relevant parameters, so the result might not be great yet :-)

Also, there is a stability issue caused by signal handlers, which is fixable. All the crashes I'm aware of that I haven't found another reason for are due to that...

Pip