Skip to content

Conversation

@kartben
Copy link
Contributor

@kartben kartben commented Mar 13, 2024

Breathe can take quite some time to generate the contents of doxygengroup:: directives and handle cross references to them.

This commit adds an option to generate empty doxygengroups in the documentation, and enables it in the make html-fast target.

make clean && make html-fast now takes about 3 min on my machine vs 16 min for a full HTML build (make clean && make html), and 8min30s for the previous version of make html-fast that only disabled devicetree bindings index generation.

Breathe can take quite some time to generate the contents of
`doxygengroup::` directives and handle cross references to them.

This commit adds an option to generate empty doxygengroups in the
documentation, and adds this option to the `make html-fast` target.

Signed-off-by: Benjamin Cabé <[email protected]>
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Ack on idea and improvement in speed (not clued up on sphinx so need someone else to check the code is good)

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

we should think (again) about abandoning breathe once and forever and focus on making Doxygen a 1st class citizen (see #39702 as an alternative). Regarding DT, a Kconfig-like solution would also solve the problem.

Comment on lines +310 to +328
if self.config.zephyr_breathe_skip_doxygen_groups:
note = nodes.attention()
note += nodes.Text(
f"Doxygen group {self.arguments[0]} was excluded from the documentation."
)

doxygengroup_node = [note]
else:
doxygengroup_node = super().run()

if self.config.zephyr_breathe_insert_related_samples:
return [RelatedCodeSamplesNode(id=self.arguments[0]), *nodes]
return [RelatedCodeSamplesNode(id=self.arguments[0]), *doxygengroup_node]
else:
return nodes
return doxygengroup_node


def setup(app):
app.add_config_value("zephyr_breathe_insert_related_samples", False, "env")
app.add_config_value("zephyr_breathe_skip_doxygen_groups", False, "env")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is overloading the Zephyr domain extension purpose. Shouldn't this be a separate extension?

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 14, 2024
@teburd
Copy link
Contributor

teburd commented May 24, 2024

There was a terrible performance bug in sphinx c domain, resulting in a horrible linear search. Effectively turning the problem of symbol reference matching into O(n^2)...

sphinx-doc/sphinx#10966

It's been solved at least for C, though not yet C++

sphinx-doc/sphinx#12162

With this change to sphinx things improve quite a lot. Sphinx still has to parse and generate mappings though for all references in RST files. Removing the c domain markers that breathe generates reduces this a good amount even more.

Using pypy further improves the situation.

With this PR, sphinx master, and pypy 3.10 docs build in 120s on my 18 core 10980xe. Prior to these changes it would take nearly 7 minutes!

7mins to 2mins is a good improvement.

I think if we look at this higher level still though, perhaps using sphinx doc which is clearly not fast (python... when is python ever fast), may be has come to the end of the line in terms of acceptable build times. Docs should be fast to build so people want to edit and update them. I got here because I was trying to do just that and was fed up with longer build times than I recall the linux kernel taking, certainly much longer than any zephyr firmware build or doxygen build.

For comparison...

doxygen build of zephyr docs takes ~12s

Would it really be so terrble to write the docs in doxygen? doxygen theme is nice now, and clearly doxygen itself is able to generate a pretty nice manual with it https://www.doxygen.nl/manual/index.html

@teburd teburd removed the Stale label May 24, 2024
@gmarull
Copy link
Member

gmarull commented May 27, 2024

There was a terrible performance bug in sphinx c domain, resulting in a horrible linear search. Effectively turning the problem of symbol reference matching into O(n^2)...

sphinx-doc/sphinx#10966

It's been solved at least for C, though not yet C++

sphinx-doc/sphinx#12162

With this change to sphinx things improve quite a lot. Sphinx still has to parse and generate mappings though for all references in RST files. Removing the c domain markers that breathe generates reduces this a good amount even more.

Using pypy further improves the situation.

With this PR, sphinx master, and pypy 3.10 docs build in 120s on my 18 core 10980xe. Prior to these changes it would take nearly 7 minutes!

7mins to 2mins is a good improvement.

I think if we look at this higher level still though, perhaps using sphinx doc which is clearly not fast (python... when is python ever fast), may be has come to the end of the line in terms of acceptable build times. Docs should be fast to build so people want to edit and update them. I got here because I was trying to do just that and was fed up with longer build times than I recall the linux kernel taking, certainly much longer than any zephyr firmware build or doxygen build.

For comparison...

doxygen build of zephyr docs takes ~12s

Would it really be so terrble to write the docs in doxygen? doxygen theme is nice now, and clearly doxygen itself is able to generate a pretty nice manual with it https://www.doxygen.nl/manual/index.html

I've advocated to stop using breathe for a long time, one can't compete with Doxygen speed. We also have tons of redundant/non-useful stuff in docs, e.g. most board and sample pages, which increase the build time for nothing.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 27, 2024
@kartben
Copy link
Contributor Author

kartben commented Aug 9, 2024

Superseded by #73671

@kartben kartben closed this Aug 9, 2024
@kartben kartben deleted the turbo_breathe branch August 16, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants