Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

>     Inhibit modification hooks when saving eieio-persistent's
>     
>     * lisp/emacs-lisp/eieio-base.el (eieio-persistent-save): Bind
>     inhibit-modification-hooks -> t.

I think this deserves an explanation (as a extensive comment explaining
why it's needed and why we think it's safe to do it here.)

Binding `inhibit-modification-hooks` is not as dangerous as binding
`inhibit-quit` but it's something we should still avoid as much
as possible.


        Stefan


> ---
>  lisp/emacs-lisp/eieio-base.el | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/eieio-base.el b/lisp/emacs-lisp/eieio-base.el
> index 2cb1f61..010a2b6 100644
> --- a/lisp/emacs-lisp/eieio-base.el
> +++ b/lisp/emacs-lisp/eieio-base.el
> @@ -473,7 +473,8 @@ instance."
>      (let* ((cfn (or file (oref this file)))
>             (default-directory (file-name-directory cfn)))
>        (cl-letf ((standard-output (current-buffer))
> -                ((oref this file)       ;FIXME: Why change it?
> +                (inhibit-modification-hooks t)
> +                ((oref this file) ;FIXME: Why change it?
>                   (if file
>                       ;; FIXME: Makes a name relative to (oref this file),
>                       ;; whereas I think it should be relative to cfn.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> >     Inhibit modification hooks when saving eieio-persistent's
> >
> >     * lisp/emacs-lisp/eieio-base.el (eieio-persistent-save): Bind
> >     inhibit-modification-hooks -> t.
>
> I think this deserves an explanation (as a extensive comment explaining
> why it's needed and why we think it's safe to do it here.)

I thought this would be obvious?  The output always goes into a fresh
temp buffer the user never sees, and the goal is to speed things up.
Sure, I can add a comment, but I don't know how I can achieve the
"extensive".

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

> I thought this would be obvious?  The output always goes into a fresh
> temp buffer the user never sees, and the goal is to speed things up.

It's far from the only code which adds things to a temp buffer, yet we
usually don't bind that var around such code because the speed
difference is usually not significant.

What kind of speed up have you noticed?
Have you profiled it to see which change-functions slow us down (maybe
we can disable them some other way, such as by using another major mode)?

Have you tried to use `combine-change-calls`?


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> What kind of speed up have you noticed?

A factor of roughly 4.

> Have you profiled it to see which change-functions slow us down

I tried, but no, with "profiler" I only saw 80 % of time wasted
somewhere, but no samples where assigned to anything.  That's why it
took an hour until I found that change functions are the culprit.  Note
that they are typically run 10000 or 10000 times or so.

I don't want to conceal that I have on-screen.el making use of
after-change hooks - but I dunno what other people might have put
there.  Having such stuff in such a low-level operation is rather
useless, and I don't know what modes people are using are putting into
after-change hooks.

> (maybe we can disable them some other way, such as by using another
> major mode)?

Other than `fundamental-mode'?

> Have you tried to use `combine-change-calls`?

No, but that's something I can try, yes.  Actually, `object-write' would
be the better place to use this, but since `combine-change-calls` needs
to know a region, and `object-write' is agnostic about where it writes
to, this doesn't seem to be possible.  Or should I just test
`standard-output'?  I guess it doesn't make sense to call
after-change-functions for every single atomic operation of
`object-write'.

Thanks,

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

> I don't want to conceal that I have on-screen.el making use of
> after-change hooks - but I dunno what other people might have put
> there.

Indeed, on-screen.el adds itself *globally* to `after-change-functions`
so it's also applied to temp buffers.  Sounds like a performance bug in
`on-screen.el`.

Do you still see the performance problem if you don't enable `on-screen`?

>> Have you tried to use `combine-change-calls`?
> No, but that's something I can try, yes.  Actually, `object-write' would
> be the better place to use this,

Not necessarily: it could be too slow (because of the cost to
enter/leave `combine-change-calls`).


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Do you still see the performance problem if you don't enable
> `on-screen`?

There are other similar "culprits" as well, all are private things.
When I turn them all off, everything is good.

> Not necessarily: it could be too slow (because of the cost to
> enter/leave `combine-change-calls`).

You mean when it's called in iteration?  That makes sense.

Ok, I'll try to use it instead of `inh-mod-hooks' in
`eieio-persistent-save'.


Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Eric Abrahamsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>     Inhibit modification hooks when saving eieio-persistent's
>>     
>>     * lisp/emacs-lisp/eieio-base.el (eieio-persistent-save): Bind
>>     inhibit-modification-hooks -> t.
>
> I think this deserves an explanation (as a extensive comment explaining
> why it's needed and why we think it's safe to do it here.)
>
> Binding `inhibit-modification-hooks` is not as dangerous as binding
> `inhibit-quit` but it's something we should still avoid as much
> as possible.

I'm curious what the objection is -- why would we *want* modification
hooks to run when writing an eieio object out to file?

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I'm curious what the objection is -- why would we *want* modification
> hooks to run when writing an eieio object out to file?

I guess just because the medicine is too strong?  `combine-change-calls'
works equally good for me.

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Indeed, on-screen.el adds itself *globally* to `after-change-functions`
> so it's also applied to temp buffers.  Sounds like a performance bug in
> `on-screen.el`.

Yes, it had been intentionally, but I think it's easy to avoid, so I'll
fix that.

Thanks,

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

>> Do you still see the performance problem if you don't enable
>> `on-screen`?
> There are other similar "culprits" as well, all are private things.
> When I turn them all off, everything is good.

Sounds like performance problems in your "culprits", then.
`before/after-change-functions` are functions that can be called *many*
times within a single command and for this reason they have to be
careful about their performance impact.

E.g. for `on-screen`, AFAICT the after-change-function is only there to
remove the highlighting when you modify the buffer (tho, AFAICT it
doesn't seem to work in my tests, so maybe I misunderstand it).

So IIUC, your after-change-function only need to be installed in those
buffers where there is highlighting to be removed.  IOW, the hook
function should be added to the buffer when you add highlighting to it,
and then removed when you remove the highlighting.  This will also
ensure that it will only be called once per command in any given buffer.

>> Not necessarily: it could be too slow (because of the cost to
>> enter/leave `combine-change-calls`).
> You mean when it's called in iteration?  That makes sense.

Another reason is that we don't know that `object-write` is only called
in temp buffers.


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

> I'm curious what the objection is -- why would we *want* modification
> hooks to run when writing an eieio object out to file?

Do you know what modification hooks are used for?
I know some uses, but not all.  So it's pretty hard to convince myself
that there really can't be any good reason why someone might want those
hooks to be run.

More importantly, `inhibit-modification-hooks` is bound here globally,
so it will affect behavior of Emacs not just in that temp buffer if some
code happens to modify other buffers during execution of this code.
That can be *anything* when you debug the code or when you run
a sufficiently interesting `object-write` method.


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Eric Abrahamsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I'm curious what the objection is -- why would we *want* modification
>> hooks to run when writing an eieio object out to file?
>
> Do you know what modification hooks are used for?

No clue! But eieio-persistent is pretty restrictive in its other
read/write processes (removing text properties, etc), so it seemed like
this would be the wrong tool to use for changing how eieio objects are
written.

> I know some uses, but not all. So it's pretty hard to convince myself
> that there really can't be any good reason why someone might want
> those hooks to be run.
>
> More importantly, `inhibit-modification-hooks` is bound here globally,
> so it will affect behavior of Emacs not just in that temp buffer if some
> code happens to modify other buffers during execution of this code.
> That can be *anything* when you debug the code or when you run
> a sufficiently interesting `object-write` method.

Yes, that's definitely a problem. Anyway, sounds like you and Michael
have found an alternate solution.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> E.g. for `on-screen`, AFAICT the after-change-function is only there to
> remove the highlighting when you modify the buffer (tho, AFAICT it
> doesn't seem to work in my tests, so maybe I misunderstand it).

Did you turn `on-screen-remove-when-edit' on (default: off)?

> So IIUC, your after-change-function only need to be installed in those
> buffers where there is highlighting to be removed.  IOW, the hook
> function should be added to the buffer when you add highlighting to it,
> and then removed when you remove the highlighting.  This will also
> ensure that it will only be called once per command in any given
> buffer.

Yes, exactly that.

> >> Not necessarily: it could be too slow (because of the cost to
> >> enter/leave `combine-change-calls`).
> > You mean when it's called in iteration?  That makes sense.
>
> Another reason is that we don't know that `object-write` is only called
> in temp buffers.

I understand that answer for `inhibit-modification-hooks', but not for
`combine-change-calls' - what is bad about combining change calls when
writing to an arbitrary buffer?

BTW, while we are on the subject, I don't understand this sentence in
the doc of the related `combine-after-change-calls' (which I don't
suggest to use):

| If BODY makes changes in the buffer, they are recorded and the
| functions on `after-change-functions' are called several times when
| BODY is finished.

Why several times - I thought this is what the thing would avoid?  And
how often is "several times"?

Thanks,

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

>> >> Not necessarily: it could be too slow (because of the cost to
>> >> enter/leave `combine-change-calls`).
>> > You mean when it's called in iteration?  That makes sense.
>> Another reason is that we don't know that `object-write` is only called
>> in temp buffers.
> I understand that answer for `inhibit-modification-hooks', but not for
> `combine-change-calls' - what is bad about combining change calls when
> writing to an arbitrary buffer?

`combine-change-calls` is a bit less blunt, but it should still be used
with care.

For example, if you don't run the buffer modification hooks in the body,
it means that the `syntax-ppss` and `syntax-propertize` won't be
properly flushed, so if you use them interspersed with buffer
modifications you may get incorrect results.

As this is a common problem, `combine-change-calls` is careful to
preserve the `syntax-ppss-flush-cache` on `before-change-functions`, but
it shows the risk of what can happen with other change-functions.

> BTW, while we are on the subject, I don't understand this sentence in
> the doc of the related `combine-after-change-calls' (which I don't
> suggest to use):
>
> | If BODY makes changes in the buffer, they are recorded and the
> | functions on `after-change-functions' are called several times when
> | BODY is finished.
>
> Why several times - I thought this is what the thing would avoid?  And
> how often is "several times"?

I think it's as many times as there were modifications, tho I haven't
looked at this code in too many years to remember for sure.
I'm not sure why it was designed that way.


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> `combine-change-calls` is a bit less blunt, but it should still be used
> with care.

After having identified the culprits in my case - all of them can be
fixed - there is not much speedup left from this particular change.
Given that, but also given that we don't know what other people might
have added there - do you think that using `combine-change-calls' in
this case is justified, Stefan?

Thanks, Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Stefan Monnier]

>> `combine-change-calls` is a bit less blunt, but it should still be used
>> with care.
> After having identified the culprits in my case - all of them can be
> fixed - there is not much speedup left from this particular change.
> Given that, but also given that we don't know what other people might
> have added there - do you think that using `combine-change-calls' in
> this case is justified, Stefan?

I hope not ;-)


        Stefan

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > After having identified the culprits in my case - all of them can be
> > fixed - there is not much speedup left from this particular change.
> > Given that, but also given that we don't know what other people might
> > have added there - do you think that using `combine-change-calls' in
> > this case is justified, Stefan?
>
> I hope not ;-)

Then Eric I think it's your turn now to check (profile) if this is at
all helpful in your case.  Could you please try that?

Thanks,

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> > After having identified the culprits in my case - all of them can be
>> > fixed - there is not much speedup left from this particular change.
>> > Given that, but also given that we don't know what other people might
>> > have added there - do you think that using `combine-change-calls' in
>> > this case is justified, Stefan?
>>
>> I hope not ;-)
>
> Then Eric I think it's your turn now to check (profile) if this is at
> all helpful in your case.  Could you please try that?

Okay, I started with the existing `inhibit-modification-hooks' edit.
Saving both the gnus registry (not many objects but they're pretty
large; file is 30M) and one of my EBDB databases (many objects but
they're quite small; file is 550K). Basically just loaded both objects
into a variable, and then repeatedly saved the object with
`eieio-persistent-save'.

;; With modification hooks uninhibited (ie, reverting Michael's commit).
(benchmark-run 100 (my-save-registry)) -> (1028.64836554 2801 537.1068782020001)
(benchmark-run 100 (my-save-ebdb)) -> (51.585404029 134 20.779424303000003)

;; With modification hooks inhibited (Emacs master).
(benchmark-run 100 (my-save-registry)) -> (989.206686569 2800 526.325465032)
(benchmark-run 100 (my-save-ebdb)) -> (63.946958194 127 23.392065172000002)

I haven't tried it with `combine-change-calls' or any of the other
speedups (gc-cons-threshold, etc), mostly because I've also been trying
to get work done. But I can test with any combination of changes, as
needed.

Eric

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> ;; With modification hooks uninhibited (ie, reverting Michael's commit).
> (benchmark-run 100 (my-save-registry)) -> (1028.64836554 2801
> 537.1068782020001)
> (benchmark-run 100 (my-save-ebdb)) -> (51.585404029 134 20.779424303000003)
>
> ;; With modification hooks inhibited (Emacs master).
> (benchmark-run 100 (my-save-registry)) -> (989.206686569 2800 526.325465032)
> (benchmark-run 100 (my-save-ebdb)) -> (63.946958194 127 23.392065172000002)

`my-save-ebdb' is slower with modification hooks inhibited?

Anyway, the time differences are quite small.

Michael.

Re: master c59e878: Inhibit modification hooks when saving eieio-persistent's

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> ;; With modification hooks uninhibited (ie, reverting Michael's commit).
>> (benchmark-run 100 (my-save-registry)) -> (1028.64836554 2801
>> 537.1068782020001)
>> (benchmark-run 100 (my-save-ebdb)) -> (51.585404029 134 20.779424303000003)
>>
>> ;; With modification hooks inhibited (Emacs master).
>> (benchmark-run 100 (my-save-registry)) -> (989.206686569 2800 526.325465032)
>> (benchmark-run 100 (my-save-ebdb)) -> (63.946958194 127 23.392065172000002)
>
> `my-save-ebdb' is slower with modification hooks inhibited?

That's why this took me so long, I decided I was confused and re-ran the
tests all over again.

Also I forgot to note:

My before-change-functions: (org-indent-notify-modified-headline org-before-change-function t syntax-ppss-flush-cache)

My after-change-functions: (jit-lock-after-change flyspell-after-change-function org-indent-refresh-maybe t)