Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache subtypes to speed up selector dispatch #1440

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Cache subtypes to speed up selector dispatch #1440

merged 2 commits into from
Oct 19, 2020

Conversation

epatters
Copy link
Contributor

@epatters epatters commented Oct 19, 2020

Addresses #1438 by caching the results of the subtypes function. This PR reduces the runtime of the Catlab docs build from ~1 hr to 10 min.

The approach taken here has the side effect that a selector cannot have more steps assigned to it after it has been dispatched at least once, but hopefully that is not a problem.

@mortenpi mortenpi added this to the 0.25.3 milestone Oct 19, 2020
@mortenpi mortenpi mentioned this pull request Oct 19, 2020
@KristofferC
Copy link
Member

Unrelated to the PR, but this whole "Selector system" feels kinda odd to me. Using dynamic dispatch and reflection for what amounts to a "do this, then do this" construct seems quite complicated and slow.

@MichaelHatherly
Copy link
Member

MichaelHatherly commented Oct 19, 2020

Using dynamic dispatch and reflection for what amounts to a "do this, then do this" construct seems quite complicated and slow.

Yeah, would be good to get rid of it. I guess it just depends on whether any users are actually making use of it for extension purposes. (Edit: from a brief gh search there doesn't appear to be any usage, so after this long not having it made use of we could safely remove it without much problem. If anyone who is using it sees this please speak up.)

@MichaelHatherly
Copy link
Member

The approach taken here has the side effect that a selector cannot have more steps assigned to it after it has been dispatched at least once, but hopefully that is not a problem.

@epatters did you also try the initial approach you outlined in #1438 (comment)?

A simple workaround would move the call sort(subtypes(T); by = order) from inside dispatch to the body of the main function expand.

@epatters
Copy link
Contributor Author

@MichaelHatherly, I was originally going to try that, but then I realized it would fragment the dispatch logic across the package. I thought it would be better to isolate the workaround where the problem occurs so that the rationale will be clear to readers of the code.

@fredrikekre fredrikekre merged commit 0962d06 into JuliaDocs:master Oct 19, 2020
@epatters epatters deleted the cache-subtypes branch October 19, 2020 18:35
@mortenpi
Copy link
Member

Does this kind of undo #620?

If anyone who is using it sees this please speak up.

DocumenterCitations.jl is an example that uses the selectors API for extending Documenter. But that shouldn't block us from improving it -- it's not, strictly, even part of the public API.

@MichaelHatherly
Copy link
Member

DocumenterCitations.jl is an example that uses the selectors API for extending Documenter.

Cool package. Looks pretty active, so probably best not to disrupt it too much if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants