Use "split" version of 'miniaudio' and Pimpl 'AudioDevice'

[vittorioromeo]

The header-only version of miniaudio is almost 4MB. This means that every translation unit that includes miniaudio.h basically copy-pastes 4MB inside itself, even though all translation units but one need the implementation.

Split version is available directly from the miniaudio repo:
https://github.com/mackron/miniaudio/tree/master/extras/miniaudio_split

[vittorioromeo]

@ChrisThrasher: what would be the proper way of compiling and linking against miniaudio.c?

[ChrisThrasher]

I measured compiling sfml-audio on master and this PR and build times were identical. It seems like modern preprocessors are really good at churning through all those megabytes of code.

master

$ hyperfine 'ninja clean && ninja sfml-audio'
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.981 s ±  0.017 s    [User: 11.487 s, System: 1.522 s]
  Range (min … max):    1.958 s …  2.009 s    10 runs

#2982

$ hyperfine 'ninja clean && ninja sfml-audio'
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.981 s ±  0.044 s    [User: 11.390 s, System: 1.500 s]
  Range (min … max):    1.901 s …  2.056 s    10 runs

[ChrisThrasher]

@ChrisThrasher: what would be the proper way of compiling and linking against miniaudio.c?

What you did #includeing the source file seems fine. A bit unconventional but it looks like it will work.

Something to keep in mind is that the way we run clang-tidy relies on analyzing all files that appear in compile_commands.json. If we add a 3rd party source file to the build (as in we actually tell CMake about it) then it will appear in that file and thus get analyzed. No 3rd party source file is going to pass all of our clang-tidy checks so that will cause CI to break. What you proposed seems to avoid that issue since the build system has no idea we added a new source file.

[vittorioromeo]

I measured compiling sfml-audio on master and this PR and build times were identical. It seems like modern preprocessors are really good at churning through all those megabytes of code.

master

$ hyperfine 'ninja clean && ninja sfml-audio'
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.981 s ±  0.017 s    [User: 11.487 s, System: 1.522 s]
  Range (min … max):    1.958 s …  2.009 s    10 runs

#2982

$ hyperfine 'ninja clean && ninja sfml-audio'
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.981 s ±  0.044 s    [User: 11.390 s, System: 1.500 s]
  Range (min … max):    1.901 s …  2.056 s    10 runs

I see a slight improvement with this PR, but YMMV. I suppose that people with slower SSDs or worse specs will benefit more from this PR. I also think you should benchmark tests and examples as well, to measure the impact on the TUs that might include headers that include miniaudio.h:

this PR 
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.809 s ±  0.020 s    [User: 5.190 s, System: 1.812 s]
  Range (min … max):    1.788 s …  1.831 s    4 runs

Benchmark 1: ninja clean && ninja
  Time (mean ± σ):      4.589 s ±  0.047 s    [User: 85.597 s, System: 21.479 s]
  Range (min … max):    4.533 s …  4.632 s    4 runs



master
Benchmark 1: ninja clean && ninja sfml-audio
  Time (mean ± σ):      1.889 s ±  0.040 s    [User: 5.218 s, System: 1.952 s]
  Range (min … max):    1.851 s …  1.944 s    4 runs

Benchmark 1: ninja clean && ninja
  Time (mean ± σ):      4.645 s ±  0.080 s    [User: 85.421 s, System: 21.772 s]
  Range (min … max):    4.535 s …  4.721 s    4 runs

[ChrisThrasher]

to measure the impact on the TUs that might include headers that include miniaudio.h:

miniaudio.h is not included in any public SFML headers which is why I ignored tests and examples.

[vittorioromeo]

to measure the impact on the TUs that might include headers that include miniaudio.h:

miniaudio.h is not included in any public SFML headers which is why I ignored tests and examples.

I apologize, you are correct. I didn't realize AudioDevice.hpp was under src/.

[eXpl0it3r]

The header-only version of miniaudio is almost 4MB. This means that every translation unit that includes miniaudio.h basically copy-pastes 4MB inside itself, even though all translation units but one need the implementation.

How does this PR change anything about this situation? Since you include a C file, isn't the outcome exactly the same?

Also, I'm against doing weird things like including source files to shave off 0.1s build time on a specific system. The analyzers are also not happy for good reasons, and I wouldn't want to silence them just for this.

[binary1248]

Besides the points already mentioned above, bear in mind that in the case of miniaudio, the maintainer explicitly mentions in the split files readme that they are derived from the single header file and not the other way around. As such, the miniaudio single header is officially the preferred way of using miniaudio if a project has the choice of choosing between both methods.

The reason why this change basically doesn't change build times is because including the single header without defining MINIAUDIO_IMPLEMENTATION is equivalent to including the split header. After preprocessing there is basically no difference for the compiler. Sure, the preprocessor has to skip most of the file, but modern operating systems would have already pulled the entire contents into RAM after the first time it was read so subsequent reads will run as fast as a memcpy. Because it is a common usage pattern, I'm sure the preprocessor also has a bunch of optimizations to skip over huge blocks of code that aren't active because of a missing #define looking for the next #else or #endif.

Whoever looks for a performance benefit to this change will not find one, because it practically doesn't exist unless you are using an operating system and compiler from the 90s.

[vittorioromeo]

New results measured with ClangBuildAnalyzer:

#
#
# WITH PR

623 ms: C:/OHWorkspace/SFML/extlibs/headers/miniaudio/miniaudio.h (included 7 times, avg 89 ms), included via:
  4x: <direct include>
  3x: MiniaudioUtils.hpp 

#
#
# WITHOUT PR

674 ms: C:/OHWorkspace/SFML/extlibs/headers/miniaudio/miniaudio.h (included 12 times, avg 56 ms), included via:
  5x: AudioDevice.hpp 
  4x: <direct include>
  3x: MiniaudioUtils.hpp 

---

How does this PR change anything about this situation? Since you include a C file, isn't the outcome exactly the same?

@eXpl0it3r: the .c file is included only in Miniaudio.cpp, while before every inclusion of miniaudio.h would copy-paste the implementation code as well.

---

Also, I'm against doing weird things like including source files to shave off 0.1s build time on a specific system.

@eXpl0it3r: I am happy to avoid the #include <miniaudio.c> and add the file to CMakeLists as part of sfml-audio's sources.

---

the maintainer explicitly mentions in the split files readme that they are derived from the single header file and not the other way around. As such, the miniaudio single header is officially the preferred way of using miniaudio if a project has the choice of choosing between both methods.

@binary1248: I had seen that, and verified that the split version was up-to-date. Considering our extlibs update workflow is manual, I don't see it as a big deal. E.g. we were behind on stb_image quite a bit before #2981.

---

The reason why this change basically doesn't change build times [...]

@binary1248: I did see a difference in my benchmarks using clang++.
#2982 (comment)

---

I've also added extra improvements to make this PR more useful.

[coveralls]

Pull Request Test Coverage Report for Build 8983900711

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 46 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on feature/use_miniaudio_split at 51.741%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Audio/AudioDevice.cpp	0	46	0.0%

Totals
Change from base Build 8964138009:	51.7%
Covered Lines:	10474
Relevant Lines:	19002

---

💛 - Coveralls

[eXpl0it3r]

How does this PR change anything about this situation? Since you include a C file, isn't the outcome exactly the same?

@eXpl0it3r: the .c file is included only in Miniaudio.cpp, while before every inclusion of miniaudio.h would copy-paste the implementation code as well.

I guess, I don't fully understand how the files are processed. I thought the preprocessor would just blindly copy paste code, not really caring if your file extension is .h or .c and we only changed one include. 🤷

Also, I'm against doing weird things like including source files to shave off 0.1s build time on a specific system.

@eXpl0it3r: I am happy to avoid the #include <miniaudio.c> and add the file to CMakeLists as part of sfml-audio's sources.

I believe the proper way to solve this would be to make mini audio its own target/lib and not just include it into SFML's source. No idea what impact that has on compilation.

New results measured with ClangBuildAnalyzer:

#
#
# WITH PR

623 ms: C:/OHWorkspace/SFML/extlibs/headers/miniaudio/miniaudio.h (included 7 times, avg 89 ms), included via:
  4x: <direct include>
  3x: MiniaudioUtils.hpp 

#
#
# WITHOUT PR

674 ms: C:/OHWorkspace/SFML/extlibs/headers/miniaudio/miniaudio.h (included 12 times, avg 56 ms), included via:
  5x: AudioDevice.hpp 
  4x: <direct include>
  3x: MiniaudioUtils.hpp 

Honestly, this still doesn't convince me at all to add all the provided changes.

I know compile time is something important to you and you enjoy fiddling with it, but I feel in this case we're really micro optimizing around a non-existent problem, with focus on one specific build platform and configuration.

At the same time we probably have a lot more open issues that we could spend our time on more productively.

I've also added extra improvements to make this PR more useful.

What do we gain with the new pimpl implementation?

[vittorioromeo]

Closing with same rationale as:
#3005 (comment)