elfeed icon indicating copy to clipboard operation
elfeed copied to clipboard

Do not need to require in a function

Open yangsheng6810 opened this issue 4 years ago • 4 comments

elfeed-show would have been required already

yangsheng6810 avatar Dec 25 '20 20:12 yangsheng6810

While true, I like that the dependency is explicit.

The awkwardness is due to circular dependencies, a trap I stepped into early without realizing the consequences. The package entrypoint is elfeed.el, and it depends on elfeed-search.el. But elfeed-search.el also depends on elfeed.el for core Elfeed functionality. Similarly, as you've seen here, elfeed-search and elfeed-show depend on each other.

What I probably should have done is made elfeed.el only the entrypoint, split out the "core" Elfeed functionality into, say, elfeed-core, then depend on everything from elfeed.el.

Though this still doesn't quite solve the circular dependency between elfeed-show and elfeed-search. I'd either need to merge them into a common source file (undesirable), or have one expose hooks on which the other registers, so that one of the dependencies is "dynamic" and breaks the loop.

skeeto avatar Jan 09 '21 20:01 skeeto

The reason I raised this PR is that every call to require will loop through current-load-list, and the overhead can be huge since elfeed-search-show-entry runs for every entry in a search. It is noticeable for me.

I am not sure I understand the dependency issue. Since elfeed.el is the only entry point, it should be OK if it requires everything else, and all other files requires nothing. By the time any function is invoked, everything should have already been loaded. Functions defined in another file can be declared with declare-function. Another (maybe not ideal) solution is to set some functions as autoload, and corresponding dependencies are pulled in just in time.

yangsheng6810 avatar Jan 09 '21 21:01 yangsheng6810

What kind of search is this? elfeed-search-show-entry is an interactive function (i.e. user facing) that Elfeed only uses as a "RET" binding from elfeed-search, and "n" and "p" in elfeed-show. It sounds like you're calling it from a custom search function that renders each entry with elfeed-show and searches within that buffer? If so, I'm surprised to hear that scanning current-load-list is so slow compared to parsing HTML and rendering it in a buffer.

I'd like to understand the use-case better.

skeeto avatar Jan 09 '21 21:01 skeeto

My fault. I confused it with some other function. Here is what happened. I was experiencing a long delay (a few seconds) after I hit "RET" on an entry from elfeed-search, before the content shows up (I am only talking about the text part, as images can take much longer to load). A quick look into the code reveals an obvious unnecessary require call. I made an advice to skip this call, and it seems I am experiencing the delay much less often. When it do happen, a quick profiling shows most of the time in auto compressing/uncompressing (I forgot the function name). While the improvements in performance may be of some other origin, I still feel it a good idea to remove it from a function called every time I open an entry.

yangsheng6810 avatar Jan 09 '21 22:01 yangsheng6810