all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: Emacs-diffs Digest, Vol 19, Issue 62
       [not found] ` <E1BdKWZ-0004NI-3s@fencepost.gnu.org>
@ 2004-06-24  7:55   ` Kim F. Storm
  2004-06-24  8:50     ` Kim F. Storm
  2004-06-25  6:28     ` Richard Stallman
  0 siblings, 2 replies; 14+ messages in thread
From: Kim F. Storm @ 2004-06-24  7:55 UTC (permalink / raw)
  Cc: emacs-devel

Richard Stallman <rms@gnu.org> writes:

> Why is this necessary?

I don't know ... anybody else have a clue?

The reason I added it is that I had SEVERE problems with emacs
misbehaving or crashing with the initial version of this patch.

One "misbehaviour" was that reading mail with Gnus suddenly would
signal not-a-list errors when going from one message to another.  I
guess that Gnus uses quite large lists to keep track of the messages I
have read -- and convert that information to/from text, so I would
guess it uses mapconcat or mapcar in that process, and thus triggers
the new code...

I then added the "dogc" stuff to the Lisp_Save_Value so that GC scans
the allocated memory.  (I'm on GNU/Linux which uses the MARK_STACK GC
method).

After adding the GC awareness, those crashes went away, so I thought I
better add GCPROs for the non-MARK_STACK case as well.

But you are right that it looks superfluous ... 

> 
>     diff -c emacs/src/fns.c:1.367 emacs/src/fns.c:1.368
>     *** emacs/src/fns.c:1.367	Tue Jun 22 13:57:00 2004
>     --- emacs/src/fns.c	Tue Jun 22 14:48:10 2004
>     ***************
>     *** 3020,3026 ****
>     --- 3020,3030 ----
> 	for (i = 1; i < nargs; i += 2)
> 	  args[i] = separator;
> 
>     +   GCPRO1 (*args);
>     +   gcpro1.nvars = nargs;
> 	ret = Fconcat (nargs, args);
>     +   UNGCPRO;
>     + 
> 	SAFE_FREE_LISP (nargs);
> 
> As far as I know, Feval can't be called from Fconcat, so GC
> cannot occur there.
> 
>     +   GCPRO1 (*args);
>     +   gcpro1.nvars = leni;
> 	ret = Flist (leni, args);
>     +   UNGCPRO;
>     + 
> 
> Same issue there.
> 
> 

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-06-24  7:55   ` Emacs-diffs Digest, Vol 19, Issue 62 Kim F. Storm
@ 2004-06-24  8:50     ` Kim F. Storm
  2004-06-30 12:09       ` Andreas Schwab
  2004-06-25  6:28     ` Richard Stallman
  1 sibling, 1 reply; 14+ messages in thread
From: Kim F. Storm @ 2004-06-24  8:50 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> Richard Stallman <rms@gnu.org> writes:
> 
> > Why is this necessary?
> 
> I don't know ... anybody else have a clue?

Actually, thinking more about this, I now realize that the GCPROs are
indeed not needed in the non-MARK_STACK case.

If they were needed, they had to be added around the calls to mapcar1,
but mapcar1 already does the necessary GCPRO of the (partial) args
array.

Since the GCPROs does nothing in the MARK_STACK case, that
also why my "dogc" change DTRT for MARK_STACK-based GC.

I'll remove the GCPROs.

> 
> The reason I added it is that I had SEVERE problems with emacs
> misbehaving or crashing with the initial version of this patch.
> 
> One "misbehaviour" was that reading mail with Gnus suddenly would
> signal not-a-list errors when going from one message to another.  I
> guess that Gnus uses quite large lists to keep track of the messages I
> have read -- and convert that information to/from text, so I would
> guess it uses mapconcat or mapcar in that process, and thus triggers
> the new code...
> 
> I then added the "dogc" stuff to the Lisp_Save_Value so that GC scans
> the allocated memory.  (I'm on GNU/Linux which uses the MARK_STACK GC
> method).
> 
> After adding the GC awareness, those crashes went away, so I thought I
> better add GCPROs for the non-MARK_STACK case as well.
> 
> But you are right that it looks superfluous ... 
> 
> > 
> >     diff -c emacs/src/fns.c:1.367 emacs/src/fns.c:1.368
> >     *** emacs/src/fns.c:1.367	Tue Jun 22 13:57:00 2004
> >     --- emacs/src/fns.c	Tue Jun 22 14:48:10 2004
> >     ***************
> >     *** 3020,3026 ****
> >     --- 3020,3030 ----
> > 	for (i = 1; i < nargs; i += 2)
> > 	  args[i] = separator;
> > 
> >     +   GCPRO1 (*args);
> >     +   gcpro1.nvars = nargs;
> > 	ret = Fconcat (nargs, args);
> >     +   UNGCPRO;
> >     + 
> > 	SAFE_FREE_LISP (nargs);
> > 
> > As far as I know, Feval can't be called from Fconcat, so GC
> > cannot occur there.
> > 
> >     +   GCPRO1 (*args);
> >     +   gcpro1.nvars = leni;
> > 	ret = Flist (leni, args);
> >     +   UNGCPRO;
> >     + 
> > 
> > Same issue there.
> > 
> > 

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-06-24  7:55   ` Emacs-diffs Digest, Vol 19, Issue 62 Kim F. Storm
  2004-06-24  8:50     ` Kim F. Storm
@ 2004-06-25  6:28     ` Richard Stallman
  1 sibling, 0 replies; 14+ messages in thread
From: Richard Stallman @ 2004-06-25  6:28 UTC (permalink / raw)
  Cc: emacs-devel

    The reason I added it is that I had SEVERE problems with emacs
    misbehaving or crashing with the initial version of this patch.

    One "misbehaviour" was that reading mail with Gnus suddenly would
    signal not-a-list errors when going from one message to another.  I
    guess that Gnus uses quite large lists to keep track of the messages I
    have read -- and convert that information to/from text, so I would
    guess it uses mapconcat or mapcar in that process, and thus triggers
    the new code...

Perhaps your other changes (adding dogc) were necessary to fix that
bug, but these two specific changes were not.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-06-24  8:50     ` Kim F. Storm
@ 2004-06-30 12:09       ` Andreas Schwab
  2004-07-06 23:50         ` Andreas Schwab
  0 siblings, 1 reply; 14+ messages in thread
From: Andreas Schwab @ 2004-06-30 12:09 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> Since the GCPROs does nothing in the MARK_STACK case, that
> also why my "dogc" change DTRT for MARK_STACK-based GC.

But is does not compile without MARK_STACK.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-06-30 12:09       ` Andreas Schwab
@ 2004-07-06 23:50         ` Andreas Schwab
  2004-07-12  9:34           ` Kim F. Storm
  0 siblings, 1 reply; 14+ messages in thread
From: Andreas Schwab @ 2004-07-06 23:50 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
>
>> Since the GCPROs does nothing in the MARK_STACK case, that
>> also why my "dogc" change DTRT for MARK_STACK-based GC.
>
> But is does not compile without MARK_STACK.

Would you please fix that?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-06 23:50         ` Andreas Schwab
@ 2004-07-12  9:34           ` Kim F. Storm
  2004-07-12 10:06             ` Andreas Schwab
  0 siblings, 1 reply; 14+ messages in thread
From: Kim F. Storm @ 2004-07-12  9:34 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> Andreas Schwab <schwab@suse.de> writes:
> 
> > storm@cua.dk (Kim F. Storm) writes:
> >
> >> Since the GCPROs does nothing in the MARK_STACK case, that
> >> also why my "dogc" change DTRT for MARK_STACK-based GC.
> >
> > But is does not compile without MARK_STACK.
> 
> Would you please fix that?
> 

Sorry for the delay.

Does this patch work?

*** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
--- alloc.c	12 Jul 2004 11:29:19 +0200	
***************
*** 5033,5038 ****
--- 5033,5039 ----
  	  break;
  
  	case Lisp_Misc_Save_Value:
+ #if GC_MARK_STACK
  	  {
  	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
  	    /* If DOGC is set, POINTER is the address of a memory
***************
*** 5045,5050 ****
--- 5046,5052 ----
  		  mark_maybe_object (*p);
  	      }
  	  }
+ #endif
  	  break;
  
  	case Lisp_Misc_Overlay:

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12  9:34           ` Kim F. Storm
@ 2004-07-12 10:06             ` Andreas Schwab
  2004-07-12 11:13               ` Kim F. Storm
  0 siblings, 1 reply; 14+ messages in thread
From: Andreas Schwab @ 2004-07-12 10:06 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> Does this patch work?
>
> *** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
> --- alloc.c	12 Jul 2004 11:29:19 +0200	
> ***************
> *** 5033,5038 ****
> --- 5033,5039 ----
>   	  break;
>   
>   	case Lisp_Misc_Save_Value:
> + #if GC_MARK_STACK
>   	  {
>   	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
>   	    /* If DOGC is set, POINTER is the address of a memory
> ***************
> *** 5045,5050 ****
> --- 5046,5052 ----
>   		  mark_maybe_object (*p);

Do you actually need mark_maybe_object?  Wouldn't mark_object be ok, given
that any use of SAVE_ALLOCA_LISP has to initialize the array anyway?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 10:06             ` Andreas Schwab
@ 2004-07-12 11:13               ` Kim F. Storm
  2004-07-12 11:39                 ` Andreas Schwab
  0 siblings, 1 reply; 14+ messages in thread
From: Kim F. Storm @ 2004-07-12 11:13 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > Does this patch work?
> >
> > *** alloc.c	23 Jun 2004 10:30:52 +0200	1.346
> > --- alloc.c	12 Jul 2004 11:29:19 +0200	
> > ***************
> > *** 5033,5038 ****
> > --- 5033,5039 ----
> >   	  break;
> >   
> >   	case Lisp_Misc_Save_Value:
> > + #if GC_MARK_STACK
> >   	  {
> >   	    register struct Lisp_Save_Value *ptr = XSAVE_VALUE (obj);
> >   	    /* If DOGC is set, POINTER is the address of a memory
> > ***************
> > *** 5045,5050 ****
> > --- 5046,5052 ----
> >   		  mark_maybe_object (*p);
> 
> Do you actually need mark_maybe_object?  Wouldn't mark_object be ok, given
> that any use of SAVE_ALLOCA_LISP has to initialize the array anyway?

The array is _not_ initialized, it is filled in gradually, and GC may
occur in-between.  So mark_maybe_object is the right thing to use.

If you look at how traditional GCPRO is done at the places that use
those arrays, you will see that GC gradually modifies the gc data as
the array elements are filled in.

Since the array allocated by SAVE_ALLOCA_LISP is usually allocated on
the stack with alloca, it's elements are usually processed by
mark_maybe_object if MARK_STACK.  So it's the right thing to use
mark_maybe_object also when the array is allocated on the heap.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 11:13               ` Kim F. Storm
@ 2004-07-12 11:39                 ` Andreas Schwab
  2004-07-12 13:12                   ` Kim F. Storm
  2004-07-12 13:13                   ` Kim F. Storm
  0 siblings, 2 replies; 14+ messages in thread
From: Andreas Schwab @ 2004-07-12 11:39 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> The array is _not_ initialized,

It is, all current uses do that (they have to due to non-MARK_STACK
architectures).  And keeping it like this is a good idea anyway.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 11:39                 ` Andreas Schwab
@ 2004-07-12 13:12                   ` Kim F. Storm
  2004-07-12 13:25                     ` Andreas Schwab
  2004-07-12 13:13                   ` Kim F. Storm
  1 sibling, 1 reply; 14+ messages in thread
From: Kim F. Storm @ 2004-07-12 13:12 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > The array is _not_ initialized,
> 
> It is, all current uses do that (they have to due to non-MARK_STACK
> architectures).  And keeping it like this is a good idea anyway.

The way I read the code, the arrays are _not_ initialized.

And no, they don't have to be initialized due to non-MARK_STACK
architectures; the code that puts data into those arrays progressively
GCPROs only the initialized part of the array.

And IMO, it is not a good idea to waste cpu cycles initializing
(large) arrays that are subsequently filled with valid data.

But if you can show me the code that INITIALIZES those arrays, I will
believe you (and remove that code :-).

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 11:39                 ` Andreas Schwab
  2004-07-12 13:12                   ` Kim F. Storm
@ 2004-07-12 13:13                   ` Kim F. Storm
  2004-07-12 13:26                     ` Andreas Schwab
  1 sibling, 1 reply; 14+ messages in thread
From: Kim F. Storm @ 2004-07-12 13:13 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > The array is _not_ initialized,
> 
> It is, all current uses do that (they have to due to non-MARK_STACK
> architectures).  And keeping it like this is a good idea anyway.
> 

As I said, it is not...

So what about testing the patch I sent you.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 13:12                   ` Kim F. Storm
@ 2004-07-12 13:25                     ` Andreas Schwab
  0 siblings, 0 replies; 14+ messages in thread
From: Andreas Schwab @ 2004-07-12 13:25 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> And no, they don't have to be initialized due to non-MARK_STACK
> architectures; the code that puts data into those arrays progressively
> GCPROs only the initialized part of the array.

You are right, I was mixing the uses of leni and nargs in mapconcat.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 13:13                   ` Kim F. Storm
@ 2004-07-12 13:26                     ` Andreas Schwab
  2004-07-12 14:23                       ` Kim F. Storm
  0 siblings, 1 reply; 14+ messages in thread
From: Andreas Schwab @ 2004-07-12 13:26 UTC (permalink / raw)
  Cc: emacs-devel

storm@cua.dk (Kim F. Storm) writes:

> So what about testing the patch I sent you.

It's obviously correct, isn't it? :-)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: Emacs-diffs Digest, Vol 19, Issue 62
  2004-07-12 13:26                     ` Andreas Schwab
@ 2004-07-12 14:23                       ` Kim F. Storm
  0 siblings, 0 replies; 14+ messages in thread
From: Kim F. Storm @ 2004-07-12 14:23 UTC (permalink / raw)
  Cc: emacs-devel

Andreas Schwab <schwab@suse.de> writes:

> storm@cua.dk (Kim F. Storm) writes:
> 
> > So what about testing the patch I sent you.
> 
> It's obviously correct, isn't it? :-)
> 

I assume it is, but OTOH, I don't know how many times I've seen (and
written) obviously correct code that didn't work :-)

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2004-07-12 14:23 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <E1BcnjN-0005AC-0I@monty-python.gnu.org>
     [not found] ` <E1BdKWZ-0004NI-3s@fencepost.gnu.org>
2004-06-24  7:55   ` Emacs-diffs Digest, Vol 19, Issue 62 Kim F. Storm
2004-06-24  8:50     ` Kim F. Storm
2004-06-30 12:09       ` Andreas Schwab
2004-07-06 23:50         ` Andreas Schwab
2004-07-12  9:34           ` Kim F. Storm
2004-07-12 10:06             ` Andreas Schwab
2004-07-12 11:13               ` Kim F. Storm
2004-07-12 11:39                 ` Andreas Schwab
2004-07-12 13:12                   ` Kim F. Storm
2004-07-12 13:25                     ` Andreas Schwab
2004-07-12 13:13                   ` Kim F. Storm
2004-07-12 13:26                     ` Andreas Schwab
2004-07-12 14:23                       ` Kim F. Storm
2004-06-25  6:28     ` Richard Stallman

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.