Skip to content

Conversation

@marc-hb
Copy link
Contributor

@marc-hb marc-hb commented Feb 8, 2019

Doxygen doesn't support incremental builds. It could be ok because it
doesn't take much time in this codebase. However it's not because it
makes old output look new which has a cascade effect on sphinx:
https://sourceforge.net/p/doxygen/mailman/message/36580807/

Make doxygen artificially smarter by saving and restoring modify
timestamps on the filesystem for doxygen output files that haven't
changed.

On my system this brings down the time to run an incremental "make
htmldocs" from 75s down to 8s (cmake -DKCONFIG_TURBO_MODE=1 used in both
cases)

This optimization speeds-up non-doxygen documentation work only.

Signed-off-by: Marc Herbert [email protected]

Doxygen doesn't support incremental builds.  It could be ok because it
doesn't take much time in this codebase.  However it's not because it
makes old output look new which has a cascade effect on sphinx:
https://sourceforge.net/p/doxygen/mailman/message/36580807/

Make doxygen artificially smarter by saving and restoring modify
timestamps on the filesystem for doxygen output files that haven't
changed.

On my system this brings down the time to run an incremental "make
htmldocs" from 75s down to 8s (cmake -DKCONFIG_TURBO_MODE=1 used in both
cases)

This optimization speeds-up non-doxygen documentation work only.

Signed-off-by: Marc Herbert <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #13159 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13159   +/-   ##
=======================================
  Coverage   48.73%   48.73%           
=======================================
  Files         315      315           
  Lines       46540    46540           
  Branches    10743    10743           
=======================================
  Hits        22679    22679           
  Misses      19349    19349           
  Partials     4512     4512

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a7b45d...e22807e. Read the comment docs.

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 8, 2019

sphinx-html was just mentioned to me. Unlike sphinx-html, this doesn't take any shortcut hence safely regenerates everything that's needed; slowly in case of some actual doxygen change and faster otherwise.

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Hey thank you! This had been in my list to look into for a while. I have no objections to this, but have you tested on Windows? I will try to give it a go myself.

@mbolivar mbolivar self-requested a review February 8, 2019 16:53
Copy link
Member

@dcpleung dcpleung left a comment

Choose a reason for hiding this comment

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

LGTM

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 8, 2019

Note there's a much simpler solution if rsync becomes an acceptable Windows requirement, no need for the new python script https://sourceforge.net/p/doxygen/mailman/message/36582383/

According to cmake's documentation, "copy_if_different" copies files and not directories recursively:
https://cmake.org/cmake/help/v3.13/manual/cmake.1.html#command-line-tool-mode
I trusted the documentation and I haven't tried.

@dbkinder
Copy link
Contributor

dbkinder commented Feb 8, 2019

The patch isn't as dramatic a performance gain on windows, but still quite significant: goes from 5 minute rebuild without this patch to 2 mins.

On a Linux box, it's worth noting that this patch gives a doc rebuild time of about 12 seconds for a full doc build (including all the configuration docs), so for developers building local documents a few times to validate changes, this is a big win for subsequent rebuilds.

@dbkinder dbkinder self-requested a review February 8, 2019 23:15
Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

👍

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 9, 2019

According to cmake's documentation, "copy_if_different" copies files and not directories recursively

I just tested cmake -E copy_if_different and I confirm it doesn't copy directories. It (silently!) ignores all directory arguments and copies files only.

So the only alternative solution left is requesting Windows users to install rsync; however there's a high risk of making Windows a better place https://www.microsoft.com/en-us/p/ubuntu/9nblggh4msv6

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 11, 2019

So this fixes the incremental build time for pure .rst changes but not yet for doxygen changes, there's another and unrelated issue when doing doxygen work.

I observed that changing 1 doxygen XML file has the exact same effect as changing all doxygen files. I investigated further and found that the sphinx extension for doxygen "breathe" wrongly believes that "everything depends on everything". I reported this unrelated breathe issue in breathe-doc/breathe#420

@carlescufi
Copy link
Member

@marc-hb rsync will never be acceptable on Windows because we support native Windows Command Prompt, not Windows Subsystems for Linux or any emulation layer like Cygwin or MSYS.

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 11, 2019

There is at least one native rsync build for Windows too: DeltaCopy https://serverfault.com/questions/35336/why-hasnt-rsync-caught-on-in-the-windows-world
(I have no idea how much pain and effort a DeltaCopy requirement would add for Windows users, mentioning just for completeness)

@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 13, 2019

I just tested this on a Macbook Pro with Mojave. It brings the time to run make htmldocs-fast SPHINXOPTS=-v from 2 minutes down to 30s (only on the second run / incremental build of course)
Using SPHINXOPTS=-v and staring at the logs and "top" shows that the 30 remaining seconds are entirely spent running doxygen, the 1 min 30 s spent in sphinx is gone. @dbkinder it would be interesting if you could repeat your Windows measurement and observe the same (task manager or resmon.exe)

@galak galak merged commit 5284231 into zephyrproject-rtos:master Feb 13, 2019
@marc-hb
Copy link
Contributor Author

marc-hb commented Feb 15, 2019

The patch isn't as dramatic a performance gain on windows, but still quite significant: goes from 5 minute rebuild without this patch to 2 mins.

I suspect most of these 2 mins are spent re-running doxygen: fix submitted in PR #13442 and tested on Linux and macOS. Curious how it fares on Windows.

@marc-hb marc-hb deleted the restore-doxy-mtime branch February 15, 2019 21:32
@marc-hb
Copy link
Contributor Author

marc-hb commented Apr 1, 2021

For the record there is - at least in theory - a simpler, one-way solution: https://sourceforge.net/p/doxygen/mailman/message/36582383/

I'm just suggesting that you use Doxygen to generate a "scratch" copy that
you never deploy (and could delete immediately after rsyncing) and using a
one-way rsync to another location where you store the "real" copy for
use/deployment/processing/etc. Sphinx would run on the "real" copy, not the
scratch one
...
Even though I modified all 3 files, rsync only copied the two with changed content. The file with no changes keeps its old timestamp.

marc-hb added a commit to marc-hb/zephyr that referenced this pull request May 23, 2023
The ability to make a one-line .rst change and quickly regenerate the
docs has been broken at least three times in the past: zephyrproject-rtos#13159, zephyrproject-rtos#36033
and zephyrproject-rtos#57624. Add a small, final check to make sure this can't happen
again.

Signed-off-by: Marc Herbert <[email protected]>
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.

6 participants