Re: [RFC, experimental] save_{excursion,restriction}

[Stefan Monnier]

>> PS: Another source of slowdown for save-excursion is the
>> unchain_marker call.
> Yes, I believe that markers needs substantial redesign too.

I'm not sure that's actually needed.  I think only overlays need such
a redesign after which markers won't be used by overlays any more and
they won't be numerous enough to warrant a redesign.

Basically, I think that overlays should live inside the balanced trees
we use for text-properties, using the "Augmented tree" approach of
http://en.wikipedia.org/wiki/Interval_tree.

>  Lisp_Object
>  save_excursion_save (void)
>  {
> -  int visible = (XBUFFER (XWINDOW (selected_window)->buffer)
> -              == current_buffer);
> -
> -  return Fcons (Fpoint_marker (),
> -             Fcons (Fcopy_marker (BVAR (current_buffer, mark), Qnil),
> -                    Fcons (visible ? Qt : Qnil,
> -                           Fcons (BVAR (current_buffer, mark_active),
> -                                  selected_window))));
> +  Lisp_Object buf;
> +  struct excursion *ex;
> +  struct window *w = XWINDOW (selected_window);
> +  struct Lisp_Marker *m = XMARKER (BVAR (current_buffer, mark));
> +
> +  ex = xmalloc (sizeof *ex);
> +  ex->window = w;
> +  ex->visible = (XBUFFER (w->buffer) == current_buffer);
> +  ex->active = !NILP (BVAR (current_buffer, mark_active));
> +
> +  /* We do not initialize type and gcmarkbit since this marker
> +     is never referenced via Lisp_Object and invisible for GC.  */
> +  INIT_MARKER (&ex->point, current_buffer, PT, PT_BYTE, 0);
> +  ex->point.type = Lisp_Misc_Marker;
> +  ex->point.next = BUF_MARKERS (current_buffer);
> +  BUF_MARKERS (current_buffer) = &ex->point;

The comment says "We do not initialize type" and then you do
"ex->point.type = Lisp_Misc_Marker;".  That sounds contradictory.

> +  /* Likewise.  Note that charpos and bytepos may be zero.  */
> +  INIT_MARKER (&ex->mark, current_buffer, m->charpos,
> +            m->bytepos, m->insertion_type);

Better not use current_buffer here, but m->buffer.

> +  ex->mark.type = Lisp_Misc_Marker;
> +  ex->mark.next = BUF_MARKERS (current_buffer);
> +  BUF_MARKERS (current_buffer) = &ex->mark;

Can't you use attach_marker or some such, rather than hand-coding the
insertion of those markers into BUF_MARKERS?

> +  ex->next = current_buffer->excursions;
> +  current_buffer->excursions = ex;
> +  XSETBUFFER (buf, current_buffer);
> +  return buf;
>  }

BTW, why use an extra `excursions' field rather than just use a new
Lisp_Object type for "struct excursion"?
I think that would be cleaner to either make them into
Lisp_Pseudovector or Lisp_Misc.

> -  /* If buffer being returned to is now deleted, avoid error */
> -  /* Otherwise could get error here while unwinding to top level
> -     and crash */
> -  /* In that case, Fmarker_buffer returns nil now.  */

Where did this test go?

> +/* Used to setup base fields of Lisp_Marker.  */
> +
> +#define INIT_MARKER(mark, buf, cpos, bpos, itype)                    \
> +  ((mark)->buffer = (buf), (mark)->charpos = (cpos),                 \
> +   (mark)->bytepos = (bpos), (mark)->insertion_type = (itype), 1)

Why "1"?


        Stefan