~pkal 1 year, 25 days ago

The author expects this is due to a redefinition of json-serialize.

There is no redefinition of json-serialize going on, because every compatibility definition checks if the function is already fboundp before defining a new function.

Could you evaluate the following expression

(condition-case nil
    (let ((inhibit-message t))
      (equal (json-parse-string "[]") nil))
  (json-unavailable t)
  (void-function t))

in emacs -Q? This is the second condition that is used to check if native json parsing is available (necessary in this form as json-available-p was only added with Emacs 28).

~pkal added bug 1 year, 25 days ago

João Távora 1 year, 25 days ago · edit

Hi Phil, Tim

There is no redefinition of json-serialize going on, because every compatibility definition checks if the function is already fboundp before defining a new function.

Well, if it's not a re-definition (isn't json-serialize in stock Emacs 27.1, the version Tim is using?), there is certainly a somewhat surprising Elisp definition, and a seemingly wrong one at that.

The problem might come from jsonrpc.el's own check for the fboundp-ness of json-serialize. IOW jsonrpc.el already has its own "compatibility" mechanism. Who knows, it might might even be inferior in some aspect to compat.el. So, if the solution is to fix jsonrpc.el -- maybe by making it require compat.el, if compat.el is in core -- then I'm for it (I authored/maintain jsonrpc.el).

João

PS: Also checking how the email workflow at SourceHut works :-) Looks nice! Writing this from Emacs, after SourceHut rejected my earlier Gmail attempt on the grounds of HTML fluff, which was quite pedagogical, I must say.

~timlod 1 year, 25 days ago

Hi both, Thanks for picking this up so quickly! I'm not currently on the computer I updated Emacs on (leading to discovery of the issue), and at work I don't want to risk updating just now. However, I'm currently also running 27.1 (Debian build), and evaluating the above expression here I get nil. I'll rerun it tomorrow on the other machine.

As for the details of this issue, I'm afraid this goes too deep into Emacs internals for me to properly understand it, so I'll leave it to you. If I can help providing stack traces or other diagnostics, just let me know!

~pkal 1 year, 25 days ago*

The issue was on Compat's end, and was fixed in the last commit. It would be much appreciated if Tim could try it before I tag a new patch release.

Well, if it's not a re-definition (isn't json-serialize in stock Emacs 27.1, the version Tim is using?)

Yes, but it might be that it was Built without JSON support, in which case Compat does provide compatibility definitions.

The problem might come from jsonrpc.el's own check for the fboundp-ness of json-serialize. IOW jsonrpc.el already has its own "compatibility" mechanism.

Ok, that makes sense. It appears that Compat tests for the existence of json-serialize before jsonprc.el, defines a substitute (what was until now not complete), making it fboundp.

Who knows, it might might even be inferior in some aspect to compat.el.

Not by that much really. Given a definition of compat--json-serialize, the compatibility macro expands to:

(when (and (condition-case nil
	       (let ((inhibit-message t))
		 (equal
		  (json-parse-string "[]")
		  nil))
	     (json-unavailable t)
	     (void-function t))
	   (not (fboundp 'json-serialize)))
  (defalias 'json-serialize #'compat--json-serialize))

So, if the solution is to fix jsonrpc.el -- maybe by making it require compat.el, if compat.el is in core -- then I'm for it (I authored/maintain jsonrpc.el).

Please note that Compat is currently not in the core, but an eventual goal is to allow core packages to make use of it. If jsonrpc.el could help with this, that would be great.

~pkal 1 year, 25 days ago

"~pkal" outgoing@sr.ht writes:

The issue was on Compat's end, and was fixed in the last commit. It would be much appreciated if Tim could try it before I tag a new patch release.

It should be noted that due to this change, an overhead is caused by the reduction process of json-serialize to json-encode that might not be of interest for libraries like jsonrpc.el. If interested, I could annotate functions that are defined by Compat with symbol properties, so that you could use what is more efficient for your use-case.

João Távora 1 year, 25 days ago · edit

"~pkal" outgoing@sr.ht writes:

The issue was on Compat's end, and was fixed in the last commit. It would be much appreciated if Tim could try it before I tag a new patch release.

Well, if it's not a re-definition (isn't json-serialize in stock Emacs 27.1, the version Tim is using?) Yes, but it might be that it was Built without JSON support, in which case Compat does provide compatibility definitions.

Interesting, yes indeed. Maybe Tim could confirm.

Please note that Compat is currently not in the core, but an eventual goal is to allow core packages to make use of it. If jsonrpc.el could help with this, that would be great.

I don't know how jsonrpc.el, which is in core, can depend on a package that isn't there.

It should be noted that due to this change, an overhead is caused by the reduction process of json-serialize to json-encode that might not be of interest for libraries like jsonrpc.el. If interested, I could annotate functions that are defined by Compat with symbol properties, so that you could use what is more efficient for your use-case.

Yes, if I understand correctly is that from jsonrpc's perspective, it's better to let it call the "old" elisp json-encode directly instead of going throw a less efficient elisp shim of json-serialize. Is that what you're saying. If yes, I think this in jsonrpc.el is simpler than annotating:

(if (and (fboundp 'json-serialize) (subrp (symbol-function 'json-serialize))) ... )

João

~pkal 1 year, 25 days ago

João Távora outgoing@sr.ht writes:

Please note that Compat is currently not in the core, but an eventual goal is to allow core packages to make use of it. If jsonrpc.el could help with this, that would be great.

I don't know how jsonrpc.el, which is in core, can depend on a package that isn't there.

One approach would be to add "compat" to the Package-Requires list, but require it with the NOERROR flag. The other would be to provide a pseudo compat feature in the core.

It should be noted that due to this change, an overhead is caused by the reduction process of json-serialize to json-encode that might not be of interest for libraries like jsonrpc.el. If interested, I could annotate functions that are defined by Compat with symbol properties, so that you could use what is more efficient for your use-case.

Yes, if I understand correctly is that from jsonrpc's perspective, it's better to let it call the "old" elisp json-encode directly instead of going throw a less efficient elisp shim of json-serialize. Is that what you're saying. If yes, I think this in jsonrpc.el is simpler than annotating:

(if (and (fboundp 'json-serialize) (subrp (symbol-function 'json-serialize))) ... )

Yes, exactly. I guess this should do the job just as well, as any Compat definitions for Elisp functions aren't sually slower than what the core provides.

That being said, I see that jsonrpc.el uses plists with keywords, right? In that case, it might be possible to detect this and avoid memory allocations. I will try this out in the next few days and report back if I improves the performance or not.

~timlod 1 year, 25 days ago

I can't check right now, but I'm almost certain I have native JSON support (simply because I installed from gnu.org, and I'd imagine you'd have to explicitly compile without support on 27+). I'll check that this weekend along with the new commit.

João Távora 1 year, 25 days ago · edit

On Thu, May 5, 2022 at 12:18 PM ~timlod outgoing@sr.ht wrote:

I can't check right now, but I'm almost certain I have native JSON support (simply because I installed from gnu.org, and I'd imagine you'd have to explicitly compile without support on 27+).

What does "install from gnu.org" mean? Did you compile your own Emacs? If you don't have libjansson on your system, Emacs will be compiled without it regardless of the default intention to use it.

João

~timlod 1 year, 25 days ago

Sorry, that was unclear. It's been a long time since the install - I assumed it was just a binary install indexed on https://www.gnu.org/savannah-checkouts/gnu/emacs/emacs.html, but checking right now, I'd assume I did the apt install as I'm on Ubuntu. I have compiled some Emacsen, but I don't think I did for the one I'm actively using. Let's not speculate though, I'll update once I'm actually on that machine!

~timlod 1 year, 24 days ago

OK - turns out my home machine indeed did not have JSON support, and I did compile it myself...thanks for pointing this out! Now, for those unfortunate ones who didn't compile with JSON support, your commit fixes the issue! Now I will upgrade to 28.1, including JSON support ;)

Thanks for figuring this one out, I guess this can be closed!

~pkal REPORTED FIXED 1 year, 23 days ago

"~timlod" outgoing@sr.ht writes:

OK - turns out my home machine indeed did not have JSON support, and I did compile it myself...thanks for pointing this out! Now, for those unfortunate ones who didn't compile with JSON support, your commit fixes the issue! Now I will upgrade to 28.1, including JSON support ;)

Thanks for figuring this one out, I guess this can be closed!

Ok, great. I will tag a new release in that case!