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

Jump to bottom

Cache Publisher objects to speed up Sphinx #10337

Merged
merged 6 commits into from May 7, 2022
Merged

Cache Publisher objects to speed up Sphinx #10337

merged 6 commits into from May 7, 2022

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Apr 9, 2022 •
edited

I don't think this is a big enough change to justify being a "feature". It does seem to be quite a big speed-up though (at least 20% faster).

Tests with builddoc.yml show 55s on 5.x and 35s on this branch (~36% faster).

Tests on the main CI (Python 3.10, ubuntu) show 211s on 5.x and 160s on this branch (~24% faster).

A

Feature or Bugfix

  • Bugfix

tk0miya
tk0miya reviewed Apr 16, 2022
View reviewed changes
sphinx/builders/__init__.py Outdated Show resolved Hide resolved
sphinx/builders/__init__.py Show resolved Hide resolved
@tk0miya tk0miya added the type:proposal a feature suggestion label Apr 16, 2022
@tk0miya tk0miya added this to the 5.0.0 milestone Apr 16, 2022
@AA-Turner
Copy link
Member Author

CI passed.

A

tk0miya
tk0miya requested changes May 2, 2022
View reviewed changes
sphinx/registry.py Show resolved Hide resolved
sphinx/registry.py Outdated
@@ -461,6 +466,41 @@ def get_envversion(self, app: "Sphinx") -> Dict[str, str]:
envversion['sphinx'] = ENV_VERSION
return envversion

def create_publisher(self, app: "Sphinx", filename: str) -> Publisher:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considers registry is a mere storage of components. So it should not have "features" as possible. So I'd like to move this sphinx.io back again.

Copy link
Member Author

@AA-Turner AA-Turner May 2, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How's the current?

A

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your update. I prefer the idea of create_publisher(). +1 for this.

tk0miya
tk0miya reviewed May 5, 2022
View reviewed changes
sphinx/io.py Outdated Show resolved Hide resolved
sphinx/io.py Outdated Show resolved Hide resolved
sphinx/registry.py Outdated
@@ -461,6 +466,41 @@ def get_envversion(self, app: "Sphinx") -> Dict[str, str]:
envversion['sphinx'] = ENV_VERSION
return envversion

def create_publisher(self, app: "Sphinx", filename: str) -> Publisher:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your update. I prefer the idea of create_publisher(). +1 for this.

tk0miya
tk0miya approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented a few, but it should not prevent merging this into the 5.0 release. So I'd like to merge this before feature freeze and refactor it after the beta release.

@AA-Turner
Copy link
Member Author

@tk0miya I merged the function into Builder.read_doc, I think that's what you meant?

what is the merit of cloning?

I added a longer comment -- Docutils' publisher reuses the settings object, but in Builder.write_doctree we edit it for pickleability. I want to update Docutils to allow more of the behaviour Sphinx has to patch to happen natively, but that will take a little time.

A

@AA-Turner
Copy link
Member Author

CI now passing.

A

tk0miya
tk0miya approved these changes May 7, 2022
View reviewed changes
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits.

sphinx/io.py Show resolved Hide resolved
sphinx/builders/__init__.py
# The settings object is reused by the Publisher for each document.
# Becuase we modify the settings object in ``write_doctree``, we
# need to ensure that each doctree has an independent copy.
doctree.settings = doctree.settings.copy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating comment. I understand what you're going. I think it's much better to move this into write_doctree().

@tk0miya
Copy link
Member

tk0miya commented May 7, 2022

Thank you for your work. This must be the best improvement in 5.0!

@tk0miya tk0miya merged commit 431caac into sphinx-doc:5.x May 7, 2022
tk0miya added a commit that referenced this pull request May 7, 2022
@tk0miya
Update CHANGES for PR #10337
58c5424
@tk0miya tk0miya mentioned this pull request May 7, 2022
Merged
@AA-Turner AA-Turner deleted the reuse-publisher branch May 30, 2022 04:15
@Mariatta Mariatta mentioned this pull request Jun 7, 2022
Closed
4 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tk0miya tk0miya tk0miya approved these changes

Assignees
No one assigned
Labels
type:proposal a feature suggestion
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AA-Turner @tk0miya