Hi, Here are 3 suggested patches. 1. Build test for cmake and run the test in gitlab-ci. 2. Disable the message box on Windows on abort that cause test hangs in CI. 3. Build time improvement by removing unnecessary includes of stdio.h in production code and change to a lighter header intrin.h -> intrin0.h (windows only). Attached screenshot of measurement but it resulted in 14% buildspeed compared to master on Windows 64 debug builds on my setup. As most improvements comes from change intrin.h the impact on other platforms will not be that much. //Marcus -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20200608/0907947b/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Build-time-improvement-for-MSVC-use-intrin0.h-instea.patch Type: application/octet-stream Size: 3665 bytes Desc: 0001-Build-time-improvement-for-MSVC-use-intrin0.h-instea.patch URL: <http://lists.xiph.org/pipermail/opus/attachments/20200608/0907947b/attachment-0003.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-Disable-message-box-when-calling-abort-.-The-message.patch Type: application/octet-stream Size: 1983 bytes Desc: 0002-Disable-message-box-when-calling-abort-.-The-message.patch URL: <http://lists.xiph.org/pipermail/opus/attachments/20200608/0907947b/attachment-0004.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: header_cleanup.PNG Type: image/png Size: 181711 bytes Desc: header_cleanup.PNG URL: <http://lists.xiph.org/pipermail/opus/attachments/20200608/0907947b/attachment-0001.png> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Build-and-run-test-for-cmake-build-in-gitlab-ci.patch Type: application/octet-stream Size: 793 bytes Desc: 0001-Build-and-run-test-for-cmake-build-in-gitlab-ci.patch URL: <http://lists.xiph.org/pipermail/opus/attachments/20200608/0907947b/attachment-0005.obj>
Hi Marcus, On 2020-06-08 01:39, Marcus Asteborg wrote:> 1. Build test for cmake and run the test in gitlab-ci. > 2. Disable the message box on Windows on abort that cause test hangs in CI. > 3. Build time improvement by removing unnecessary includes of stdio.h > in production code and change to a lighter header intrin.h -> > intrin0.h (windows only). Attached screenshot of measurement but it > resulted in 14% buildspeed compared to master on Windows 64 debug > builds on my setup. As most improvements comes from change intrin.h > the impact on other platforms will not be that much.I need to have a closer look, but patches 2 and 3 look pretty reasonable. For patch 1, can you explain a bit more what it does (to someone familiar with neither cmake nor gitlab-ci). Cheers, Jean-Marc
The current CI script just checks if the build succeeds or fails against each PR/commit, this change enables it to also run tests and report the result. On Thu, Jun 11, 2020 at 12:50 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> For patch 1, can you explain a bit more what it does (to > someone familiar with neither cmake nor gitlab-ci). >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20200611/5d0270a9/attachment.html>
The cmake changes for gitlab-ci look good to me. r+ for that patch. -r
On 2020-06-08 01:39, Marcus Asteborg wrote:> 1. Build test for cmake and run the test in gitlab-ci.OK, merged.> 2. Disable the message box on Windows on abort that cause test hangs in CI.Merged too.> 3. Build time improvement by removing unnecessary includes of stdio.h > in production code and change to a lighter header intrin.h -> > intrin0.h (windows only). Attached screenshot of measurement but it > resulted in 14% buildspeed compared to master on Windows 64 debug > builds on my setup. As most improvements comes from change intrin.h > the impact on other platforms will not be that much.I'm having a bit of an issue with the change to debug.h. I'm not sure exactly what you're trying to do, but the #ifdef HAVE_CONFIG_H #include "config.h" #endif should always be there (unconditionally) at the top of C files. Any reason you want to move an include before it and make it conditional? Cheers, Jean-Marc
3, no good catch attached is an updated patch //Marcus ________________________________ From: Jean-Marc Valin <jmvalin at jmvalin.ca> Sent: Thursday, June 11, 2020 10:49 To: Marcus Asteborg <xnorpx at outlook.com>; opus at xiph.org <opus at xiph.org> Subject: Re: [opus] Misc patches On 2020-06-08 01:39, Marcus Asteborg wrote:> 1. Build test for cmake and run the test in gitlab-ci.OK, merged.> 2. Disable the message box on Windows on abort that cause test hangs in CI.Merged too.> 3. Build time improvement by removing unnecessary includes of stdio.h > in production code and change to a lighter header intrin.h -> > intrin0.h (windows only). Attached screenshot of measurement but it > resulted in 14% buildspeed compared to master on Windows 64 debug > builds on my setup. As most improvements comes from change intrin.h > the impact on other platforms will not be that much.I'm having a bit of an issue with the change to debug.h. I'm not sure exactly what you're trying to do, but the #ifdef HAVE_CONFIG_H #include "config.h" #endif should always be there (unconditionally) at the top of C files. Any reason you want to move an include before it and make it conditional? Cheers, Jean-Marc -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20200611/1315ac8d/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Build-time-improvement-for-MSVC-use-intrin0.h-instea.patch Type: application/octet-stream Size: 3450 bytes Desc: 0001-Build-time-improvement-for-MSVC-use-intrin0.h-instea.patch URL: <http://lists.xiph.org/pipermail/opus/attachments/20200611/1315ac8d/attachment-0001.obj>
Speaking of needing more complete ci feedback, the intrin0.h patch broke the appveyor build:> Microsoft (R) Build Engine version 14.0.25420.1 > [...] > :\projects\opus\celt\ecintrin.h(53): fatal error C1083: Cannot openinclude file: 'intrin0.h': No such file or directory [C:\projects\opus\win32\VS2015\opus.vcxproj] https://ci.appveyor.com/project/rillian/opus/builds/33474422 Is this an extra feature the appveyor environment doesn't apply, or is the version guard incorrect? -r On Mon, 2020-06-08 at 05:39 +0000, Marcus Asteborg wrote:> Hi, > > Here are 3 suggested patches. > Build test for cmake and run the test in gitlab-ci. > Disable the message box on Windows on abort that cause test hangs in > CI. > Build time improvement by removing unnecessary includes of stdio.h in > production code and change to a lighter header intrin.h -> intrin0.h > (windows only). Attached screenshot of measurement but it resulted in > 14% buildspeed compared to master on Windows 64 debug builds on my > setup. As most improvements comes from change intrin.h the impact on > other platforms will not be that much. > //Marcus > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus
Sorry about that, let me check the correct version for the intrin0.h include guard. //Marcus ________________________________ From: Ralph Giles <giles at thaumas.net> Sent: Thursday, June 11, 2020 19:31 To: Marcus Asteborg <xnorpx at outlook.com>; opus at xiph.org <opus at xiph.org> Subject: Re: [opus] Misc patches Speaking of needing more complete ci feedback, the intrin0.h patch broke the appveyor build:> Microsoft (R) Build Engine version 14.0.25420.1 > [...] > :\projects\opus\celt\ecintrin.h(53): fatal error C1083: Cannot openinclude file: 'intrin0.h': No such file or directory [C:\projects\opus\win32\VS2015\opus.vcxproj] https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fci.appveyor.com%2Fproject%2Frillian%2Fopus%2Fbuilds%2F33474422&data=02%7C01%7C%7C4162e72343444c9c310608d80e78ad01%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637275258756998152&sdata=1zMyaftA7NjAXL52YRlQtwyNG3Xaen2U7kngJ%2F8KZtU%3D&reserved=0 Is this an extra feature the appveyor environment doesn't apply, or is the version guard incorrect? -r On Mon, 2020-06-08 at 05:39 +0000, Marcus Asteborg wrote:> Hi, > > Here are 3 suggested patches. > Build test for cmake and run the test in gitlab-ci. > Disable the message box on Windows on abort that cause test hangs in > CI. > Build time improvement by removing unnecessary includes of stdio.h in > production code and change to a lighter header intrin.h -> intrin0.h > (windows only). Attached screenshot of measurement but it resulted in > 14% buildspeed compared to master on Windows 64 debug builds on my > setup. As most improvements comes from change intrin.h the impact on > other platforms will not be that much. > //Marcus > _______________________________________________ > opus mailing list > opus at xiph.org > https://nam10.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.xiph.org%2Fmailman%2Flistinfo%2Fopus&data=02%7C01%7C%7C4162e72343444c9c310608d80e78ad01%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637275258757008148&sdata=wrR6%2FwZJoaV%2F7s36m3MglWx41ntOa3ZGXRwQh%2BU7cDU%3D&reserved=0-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20200612/d563174e/attachment.html>