outline/allout/overlay performance (was: existing work on TODO items)

outline/allout/overlay performance (was: existing work on TODO items)

[Stefan Monnier]

> i think i've surmounted the performance problem i was seeing, by
> better stitching together the overlays.

Good news.

> where i'm getting to is a version of allout that uses a routine which
> flags text for invisibility that is very like the one in outline-mode -
> `outline-flag-region' - but uses a different routine to reconcile the
> presentation after isearches - the `isearch-open-invisible' property.
> i may want to use a special `isearch-open-invisible-temporary' property,
> as well.

You may want to check interaction with reveal-mode as well.

> these choices are hard-coded in the current `outline-flag-region', so
> i can't use it as stands.  i could suggest a slightly modified version
> which open-codes these choices so "derivative" outline modes can use it
> directly while tailoring these aspects for their specific purposes.  does
> that seem like a reasonable way to go?

I don't understand why outline-flag-region should have anything to do with
the implementation of an isearch-open-invisible(-temporary) function, so I'm
probably not understanding you well, but improving outline-flag-region so it
stitches overlays together sounds like a good idea.
Maybe it could be made into a generic function, much like remove-overlays
and moved to subr.el.


        Stefan

Re: outline/allout/overlay performance (was: existing work on TODO items)

[Ken Manheimer]

On 1/11/06, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > i think i've surmounted the performance problem i was seeing, by
> > better stitching together the overlays.
>
> Good news.

but maybe premature - the behaviour "came back".  i will have to
carefully implement the stitching together stuff before coming to a
conclusion about viability.

> [...]
> > these choices are hard-coded in the current `outline-flag-region', so
> > i can't use it as stands.  i could suggest a slightly modified version
> > which open-codes these choices so "derivative" outline modes can use it
> > directly while tailoring these aspects for their specific purposes.  does
> > that seem like a reasonable way to go?
>
> I don't understand why outline-flag-region should have anything to do with
> the implementation of an isearch-open-invisible(-temporary) function, so I'm
> probably not understanding you well, but improving outline-flag-region so it

the function is registered with the overlay, as a property - that's
why the current outline-flag-region sets it.  i want to make the
function it registers with the overlay parameterized - eg, a mode
local variable - to be registered if set.  outline-mode's current one
(`outline-isearch-open-invisible') will be used for outline-mode, and
allout will have its own.  other modes could, too.

> stitches overlays together sounds like a good idea.
> Maybe it could be made into a generic function, much like remove-overlays
> and moved to subr.el.

i'll take a look at remove-overlays, when i get to that point.

thanks!

ken manheimer
ken.manheimer@gmail.com

Re: outline/allout/overlay performance

[Stefan Monnier]

>> > i think i've surmounted the performance problem i was seeing, by
>> > better stitching together the overlays.
>> Good news.
> but maybe premature - the behaviour "came back".  i will have to
> carefully implement the stitching together stuff before coming to a
> conclusion about viability.

>> [...]
>> > these choices are hard-coded in the current `outline-flag-region', so
>> > i can't use it as stands.  i could suggest a slightly modified version
>> > which open-codes these choices so "derivative" outline modes can use it
>> > directly while tailoring these aspects for their specific purposes.  does
>> > that seem like a reasonable way to go?
>> 
>> I don't understand why outline-flag-region should have anything to do with
>> the implementation of an isearch-open-invisible(-temporary) function, so I'm
>> probably not understanding you well, but improving outline-flag-region so it

> the function is registered with the overlay, as a property - that's
> why the current outline-flag-region sets it.  i want to make the
> function it registers with the overlay parameterized - eg, a mode
> local variable - to be registered if set.  outline-mode's current one
> (`outline-isearch-open-invisible') will be used for outline-mode, and
> allout will have its own.  other modes could, too.

Actually, some other modes using similar functions add a few more
properties, so I think it makes sense to just add a &rest parameter to
outline-flag-region.

As for using a buffer-local variable for outline-isearch-open-invisible, it
assumes only one major/minor mode will use it at a time.  In reveal.el
I chose to allow a reveal-toggle-invisible property on the overlay or on the
symbol used for invisibility.  So in outline.el rather than adding the
property to every single overlay, I just do

  (put 'outline 'reveal-toggle-invisible 'outline-reveal-toggle-invisible)

and reveal.el finds it with

  (get (overlay-get ol 'invisible) 'reveal-toggle-invisible)

Maybe isearch (c|sh)ould be changed in a similar way.

>> stitches overlays together sounds like a good idea.
>> Maybe it could be made into a generic function, much like remove-overlays
>> and moved to subr.el.
> i'll take a look at remove-overlays, when i get to that point.

All I was thinking is to add a function like

    (add-overlay-props FROM TO &rest PROPS)

which either creates a new overlay with the specified props or moves
existing overlays if there's already an overlay around with the right
properties, thus trying to minimize the number of overlays in the buffer.
This said, it's probably more important to work on the C code to try and
remove the O(n) behavior.  And even if it's not removed everywhere, I can't
think of any reason why cursor movement should suffer from this
O(n) behavior.


        Stefan

Re: outline/allout/overlay performance

[Ken Manheimer]

it looks like i was raising a false alarm - the overlays were not the
problem, and in fact there was no fragmentation of them.  i made a
quick consolidating function, only to discover (1) there was no
improvement, and (2) actually assessing the extent of the overlays
without the consolidation showed that they were not fragmenting.  this
lead me to suspect my environment, and i discovered that a
long-line-highlighting custom package i was using (vvb-mode) was the
problem - disabling it totally eliminated any delays.  in fact,

the side effect of this little adventure is that i'm pretty far along
with the conversion to invisibility-overlays, i think the hard things
are done (several niggly boundary conditions, isearch fanciness), but
will need to shake out the odds and ends, and then work out the common
functionality with outline-mode, as stefan and i were discussing.

sorry about the false alarm!

ken manheimer
ken.manheimer@gmail.com

On 1/11/06, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >> > i think i've surmounted the performance problem i was seeing, by
> >> > better stitching together the overlays.
> >> Good news.
> > but maybe premature - the behaviour "came back".  i will have to
> > carefully implement the stitching together stuff before coming to a
> > conclusion about viability.
>
> >> [...]
> >> > these choices are hard-coded in the current `outline-flag-region', so
> >> > i can't use it as stands.  i could suggest a slightly modified version
> >> > which open-codes these choices so "derivative" outline modes can use it
> >> > directly while tailoring these aspects for their specific purposes.  does
> >> > that seem like a reasonable way to go?
> >>
> >> I don't understand why outline-flag-region should have anything to do with
> >> the implementation of an isearch-open-invisible(-temporary) function, so I'm
> >> probably not understanding you well, but improving outline-flag-region so it
>
> > the function is registered with the overlay, as a property - that's
> > why the current outline-flag-region sets it.  i want to make the
> > function it registers with the overlay parameterized - eg, a mode
> > local variable - to be registered if set.  outline-mode's current one
> > (`outline-isearch-open-invisible') will be used for outline-mode, and
> > allout will have its own.  other modes could, too.
>
> Actually, some other modes using similar functions add a few more
> properties, so I think it makes sense to just add a &rest parameter to
> outline-flag-region.
>
> As for using a buffer-local variable for outline-isearch-open-invisible, it
> assumes only one major/minor mode will use it at a time.  In reveal.el
> I chose to allow a reveal-toggle-invisible property on the overlay or on the
> symbol used for invisibility.  So in outline.el rather than adding the
> property to every single overlay, I just do
>
>   (put 'outline 'reveal-toggle-invisible 'outline-reveal-toggle-invisible)
>
> and reveal.el finds it with
>
>   (get (overlay-get ol 'invisible) 'reveal-toggle-invisible)
>
> Maybe isearch (c|sh)ould be changed in a similar way.
>
> >> stitches overlays together sounds like a good idea.
> >> Maybe it could be made into a generic function, much like remove-overlays
> >> and moved to subr.el.
> > i'll take a look at remove-overlays, when i get to that point.
>
> All I was thinking is to add a function like
>
>     (add-overlay-props FROM TO &rest PROPS)
>
> which either creates a new overlay with the specified props or moves
> existing overlays if there's already an overlay around with the right
> properties, thus trying to minimize the number of overlays in the buffer.
> This said, it's probably more important to work on the C code to try and
> remove the O(n) behavior.  And even if it's not removed everywhere, I can't
> think of any reason why cursor movement should suffer from this
> O(n) behavior.
>
>
>         Stefan
>

Re: outline/allout/overlay performance

[Thien-Thi Nguyen]

Ken Manheimer <ken.manheimer@gmail.com> writes:

> and then work out the common functionality with outline-mode, as
> stefan and i were discussing.

for congruence w/ hideshow, please consider including something
similar, in terms of calling convention, to `hs-set-up-overlay'
value (see progmodes/hideshow.el).

if congruence is not important, then never mind...

thi

Re: outline/allout/overlay performance

[Ken Manheimer]

On 12 Jan 2006 08:56:48 -0500, Thien-Thi Nguyen <ttn@gnu.org> wrote:
> Ken Manheimer <ken.manheimer@gmail.com> writes:
>
> > and then work out the common functionality with outline-mode, as
> > stefan and i were discussing.
>
> for congruence w/ hideshow, please consider including something
> similar, in terms of calling convention, to `hs-set-up-overlay'
> value (see progmodes/hideshow.el).
>
> if congruence is not important, then never mind...

that looks like a nice feature.  i guess you're suggesting that
outline-flag-region could enhance the overlays it sets with the
presence of some customization variable.  for now, however, i'm taking
a minimalist approach and sidestepping generalization of
outline-flag-region - using it as is, and employing defadvice so that
an allout-specific isearch-open-invisible function in allout-mode is
in allout-mode.

ken manheimer
ken.manheimer@gmail.com

Re: outline/allout/overlay performance

[Ken Manheimer]

i've made the transition to directly using outline-flag-region from
allout mode (rather than allout having it's own allout-flag-region),
and am using outline-flag-region as it stands.  to enable allout's
custom isearch-open-invisible behavior, i'm defadvicing
`outline-isearch-open-invisible' (which is what outline-flag-regexp
assigns as the isearch-open-invisible property on the overlays) so
that allout's preferred function, `allout-show-to-offshoot' is run,
instead, when the outline is in allout-mode.  this works very nicely!

more generally, i'm getting the distinct impression that there are
more than a few generalization issues in this suppression-of-detail
reveal/conceal realm, that (2) deserve comprehensive attention, and
(2) are of scope well beyond the particular things i have time for at
the moment.

that said, i do have some responses to your particular suggestions.

On 1/11/06, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> > the function is registered with the overlay, as a property - that's
> > why the current outline-flag-region sets it.  i want to make the
> > function it registers with the overlay parameterized - eg, a mode
> > local variable - to be registered if set.  outline-mode's current one
> > (`outline-isearch-open-invisible') will be used for outline-mode, and
> > allout will have its own.  other modes could, too.
>
> Actually, some other modes using similar functions add a few more
> properties, so I think it makes sense to just add a &rest parameter to
> outline-flag-region.

i don't like the idea of having every call to outline-flag-region pass
in generic mode parameters every time the function is called.  such
incidental concerns can pile up, obscuring the specific business
relevant to the actual call, complicating programmers lives with
incidental things to remember in every call.

i don't know the exact right mechanism to use for such concerns, but
hook-function lists seem like the kind of thing - though we're
probably talking about things that belong in particular modes.  that
suggests properties on some hook variable, qualified with the mode
where they belong or put in an 'all' bucket if they want to be used
everywhere.

> As for using a buffer-local variable for outline-isearch-open-invisible, it
> assumes only one major/minor mode will use it at a time.  In reveal.el
> I chose to allow a reveal-toggle-invisible property on the overlay or on the
> symbol used for invisibility.  So in outline.el rather than adding the
> property to every single overlay, I just do
>
>   (put 'outline 'reveal-toggle-invisible 'outline-reveal-toggle-invisible)
>
> and reveal.el finds it with
>
>   (get (overlay-get ol 'invisible) 'reveal-toggle-invisible)
>
> Maybe isearch (c|sh)ould be changed in a similar way.

this sounds good - i may be describing something like it, above.

> >> stitches overlays together sounds like a good idea.
> >> Maybe it could be made into a generic function, much like remove-overlays
> >> and moved to subr.el.
> > i'll take a look at remove-overlays, when i get to that point.
>
> All I was thinking is to add a function like
>
>     (add-overlay-props FROM TO &rest PROPS)
>
> which either creates a new overlay with the specified props or moves
> existing overlays if there's already an overlay around with the right
> properties, thus trying to minimize the number of overlays in the buffer.
> This said, it's probably more important to work on the C code to try and
> remove the O(n) behavior.  And even if it's not removed everywhere, I can't
> think of any reason why cursor movement should suffer from this
> O(n) behavior.

fortunately, it doesn't look like overlays need that consolidation.  i
don't know how they're implemented, but it seems like they
"consolidated" on their own.

i thank you (and everyone else who've provided clues) for helping
guide me.  i think i have a good approach at this point - the advice
mechnism scales, since others can similarly advice-wrap as needed.  it
depends on continuing use of the particular
outline-isearch-open-invisible function, but any approach is going to
establish some such dependencies...

ken
ken.manheimer@gmail.com

Re: outline/allout/overlay performance

[Thien-Thi Nguyen]

Ken Manheimer <ken.manheimer@gmail.com> writes:

> that looks like a nice feature.  i guess you're suggesting that
> outline-flag-region could enhance the overlays it sets with the
> presence of some customization variable.

close.  whether or not `outline-flag-region' enhances the overlays
is not my point.  rather, that the method for user enhancement of
those overlays be similar enough so that (ideally) the function
set as the value of one of these variables could be also used as
the value of the other.

> for now, however, i'm taking a minimalist approach and sidestepping
> generalization of outline-flag-region - using it as is, and employing
> defadvice so that an allout-specific isearch-open-invisible function
> in allout-mode is in allout-mode.

IMHO, congruence is a type of long-term minimalism, so we are not so far
apart aesthetically as much as temporally...

i'm not suggesting generalization of anything.  i'm suggesting the
conventionalization of `make-overlay' usage in similar (conceptually
speaking) contexts.  already, hideshow and outline share similar names
for similar operations, similar keybindings when using outline as a
minor mode, and similar algorithms for setting and tracking parent-child
relationships (this last assertion is a guess -- i suspect the set of
"tree ops" is not so large as to support wild variability).

thi

Re: outline/allout/overlay performance

[Stefan Monnier]

> i've made the transition to directly using outline-flag-region from
> allout mode (rather than allout having it's own allout-flag-region),
> and am using outline-flag-region as it stands.  to enable allout's
> custom isearch-open-invisible behavior, i'm defadvicing
> `outline-isearch-open-invisible' (which is what outline-flag-regexp
> assigns as the isearch-open-invisible property on the overlays) so
> that allout's preferred function, `allout-show-to-offshoot' is run,
> instead, when the outline is in allout-mode.  this works very nicely!

But doesn't work for reveal-mode which doesn't use
outline-isearch-open-invisible.

If all you reuse from outline is outline-flag-region, then it really doesn't
justify a defadvice.  Just copy outline-flag-region into allout-flag-region
and modify it: of the 6 lines, one is obsolete and one should be changed
(to avoid the ugliness of defavice).  Also having your own copy will allow
you to use `allout' as the symbol to add to the invisibility-spec, which
is cleaner.

> fortunately, it doesn't look like overlays need that consolidation.  i
> don't know how they're implemented, but it seems like they
> "consolidated" on their own.

Yes, I tried to "consolidate" them in outline as well and then realized that
(if you think about it, rather than code blindly, it's pretty obvious)
there's never any opportunity for consolidation because the area just
before/after the hidden text is always visible (it's a heading).


        Stefan

Re: outline/allout/overlay performance

[Kim F. Storm]

Ken Manheimer <ken.manheimer@gmail.com> writes:

> to enable allout's
> custom isearch-open-invisible behavior, i'm defadvicing
> `outline-isearch-open-invisible'

In general, we don't want packages included with emacs to use
defadvice.

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

Re: outline/allout/overlay performance

[Ken Manheimer]

On 1/13/06, Kim F. Storm <storm@cua.dk> wrote:

> Ken Manheimer <ken.manheimer@gmail.com> writes:
>
> > to enable allout's
> > custom isearch-open-invisible behavior, i'm defadvicing
> > `outline-isearch-open-invisible'
>
> In general, we don't want packages included with emacs to use
> defadvice.

ok.  i guess i'll do as stefan suggested, and give allout its' own
-flag-region routine, etc.

(there are attractive reasons to have allout use outline.el's basic
routines, but modifying outline.el to provide for this consideration
would mean depending on a new version of outline.el, making it yet
harder to use allout in older emacs.  i, myself, use some systems that
are for now held at some older emacs versions, so don't want to go
that route.)

ken manheimer
ken.manheimer@gmail.com