Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

[Stefan Monnier]

> +(eval-and-compile
> +  (if (< emacs-major-version 25)
> +      (defsubst exwm-input--unread-event (event)
> +        (setq unread-command-events
> +              (append unread-command-events (list event))))
> +    (defsubst exwm-input--unread-event (event)
> +      (setq unread-command-events
> +            (append unread-command-events `((t . ,event)))))))

This ends up choosing the version of the code at the time it's compiled
rather than at the time it's executed (since this is a defsubst and the
version chosen at compile time will end up being inlined everywhere).

I'd advise against using defsubst here (performance is probably of no
importance compared to the time it will take to process the event).

If you don't use defsubst, then you also don't need eval-and-compile, so
I'd recommend:

    (defalias 'exwm-input--unread-event
      (if (< emacs-major-version 25)
          (lambda (event)
            (setq unread-command-events
                  (append unread-command-events (list event))))
        (lambda (event)
          (setq unread-command-events
                (append unread-command-events `((t . ,event)))))))

and while I'm here, I wonder why you use `append` instead of `push`,
i.e. why you add the event to the end rather than to the beginning of
the queue.

The content of the queue is "the event we haven't processed yet", so in
order to do something akin to rerunning the current event, you usually
want to put at the beginning of the queue.


        Stefan

Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

[Chris Feng]

>> +(eval-and-compile
>> +  (if (< emacs-major-version 25)
>> +      (defsubst exwm-input--unread-event (event)
>> +        (setq unread-command-events
>> +              (append unread-command-events (list event))))
>> +    (defsubst exwm-input--unread-event (event)
>> +      (setq unread-command-events
>> +            (append unread-command-events `((t . ,event)))))))
>
> This ends up choosing the version of the code at the time it's compiled
> rather than at the time it's executed (since this is a defsubst and the
> version chosen at compile time will end up being inlined everywhere).
>
> I'd advise against using defsubst here (performance is probably of no
> importance compared to the time it will take to process the event).

My concern was that the function would get called very frequently (on
every key event in certain circumstances) so I made them inline.  As
for your the problem you pointed out, my understanding is that
bytecodes for Emacs 24 and 25 are largely not compatible, at least for
this package which heavily relies on EIEIO.  So even if we can choose
between the two functions at run time the compiled code still won't
run correctly.

> and while I'm here, I wonder why you use `append` instead of `push`,
> i.e. why you add the event to the end rather than to the beginning of
> the queue.
>
> The content of the queue is "the event we haven't processed yet", so in
> order to do something akin to rerunning the current event, you usually
> want to put at the beginning of the queue.

The events are received directly from X server (rather than Emacs) in
chronological order so I think it makes sense to append them to
`unread-command-events', in case previous events added not processed
timely.

Also there're related bugs with `unread-command-events' (bug#23980).
Could you take a look?  I hope it's not too late to fix them for
emacs-25.

Chris

Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

[Stefan Monnier]

> My concern was that the function would get called very frequently (on
> every key event in certain circumstances) so I made them inline.

I don't think "every key event" (human scale) can be "very frequently"
at the computer's scale.  And in any case, the time to process the event
later on will completely dwarf this.

> As for your the problem you pointed out, my understanding is that
> bytecodes for Emacs 24 and 25 are largely not compatible, at least for
> this package which heavily relies on EIEIO.  So even if we can choose
> between the two functions at run time the compiled code still won't
> run correctly.

Oh, indeed if it uses EIEIO it's quite possible that recompilation will
be needed anyway.

> The events are received directly from X server (rather than Emacs) in
> chronological order so I think it makes sense to append them to
> `unread-command-events', in case previous events added not processed
> timely.

Say you have (X Y Z) in unread-command-events, and Emacs comes takes
X out and processes it.  If during processing you push the new event
A at the end, that means it'll be processed in a different order than if
Y and Z had been received a bit later.

The right choice depends on the nature of the event you're pushing (A)
and its relationship to the current event (X), so it has to be decided
on a case-by-case basis according to the specific details.

So maybe that's indeed the behavior you want (and I don't know nearly
enough about the events you're manipulating to be able to tell), but
usually in my experience you want to push to the front.

> Also there're related bugs with `unread-command-events' (bug#23980).
> Could you take a look?

I'll take a look,


        Stefan

Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

[Chris Feng]

>> My concern was that the function would get called very frequently (on
>> every key event in certain circumstances) so I made them inline.
>
> I don't think "every key event" (human scale) can be "very frequently"
> at the computer's scale.  And in any case, the time to process the event
> later on will completely dwarf this.
>
>> As for your the problem you pointed out, my understanding is that
>> bytecodes for Emacs 24 and 25 are largely not compatible, at least for
>> this package which heavily relies on EIEIO.  So even if we can choose
>> between the two functions at run time the compiled code still won't
>> run correctly.
>
> Oh, indeed if it uses EIEIO it's quite possible that recompilation will
> be needed anyway.

Yes, performance is technically not an issue here.  But considering
that recompilation is always required, there's no need to change the
code.

>> The events are received directly from X server (rather than Emacs) in
>> chronological order so I think it makes sense to append them to
>> `unread-command-events', in case previous events added not processed
>> timely.
>
> Say you have (X Y Z) in unread-command-events, and Emacs comes takes
> X out and processes it.  If during processing you push the new event
> A at the end, that means it'll be processed in a different order than if
> Y and Z had been received a bit later.
>
> The right choice depends on the nature of the event you're pushing (A)
> and its relationship to the current event (X), so it has to be decided
> on a case-by-case basis according to the specific details.
>
> So maybe that's indeed the behavior you want (and I don't know nearly
> enough about the events you're manipulating to be able to tell), but
> usually in my experience you want to push to the front.

The key events are generated by the user and are received through a
subprocess in a non-preemptive way, so they're always processed in a
first-come, first-served fashion.  `unread-command-events' is used as
a FIFO here.

>> Also there're related bugs with `unread-command-events' (bug#23980).
>> Could you take a look?
>
> I'll take a look,

Thanks!

Chris

Re: [elpa] externals/exwm 0b8a373: Fix a `unread-command-events' issue for Emacs 24

[Stefan Monnier]

> The key events are generated by the user and are received through a
> subprocess in a non-preemptive way, so they're always processed in a
> first-come, first-served fashion.  `unread-command-events' is used as
> a FIFO here.

Makes sense, thanks,


        Stefan