Paths

Table of Contentst

[msvc] Reduce locales build-time.
AbandonedPublic
Actions

Authored by Ray Molenkamp (LazyDodo) on Apr 4 2017, 11:20 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Martijn Berger (juicyfruit)
Bastien Montagne (mont29)
Campbell Barton (campbellbarton)

Summary

The locales sub-project is the 3rd slowest build in the debug configuration coming in at

2013/x64/debug: 81.7 seconds
2015/x64/debug: 116.14 seconds
2013/x64/release: 4.92 seconds
2015/x64/release: 4.63 seconds

There's quite a disconnect in the debug/release configurations, main cause of the slowdown is the excessive error checking in msvc's stl headers combined with it being kinda heavy on the memory allocation side of things (even if you build with /O2, the debug crt still keeps the total build time of the project over ~30 seconds)

This patch builds a release copy of msgfmt even for debug builds (on windows only, nothing changes for the other platforms). We rarely update this code so there really is no reason to punish developers by tagging on nearly 2 minutes of build time for every full build they do.

Diff Detail

Event Timeline

Ray Molenkamp (LazyDodo) created this revision.Apr 4 2017, 11:20 PM

I am not happy at all with such a tricks to force particular targets to be release. If one requests Blender to be in debug mode, it should be so. Unrelated how often one debugs the code.

The same exact code needs about 4sec on Linux in debug mode to generate all locales, so there should be a way to optimize debug builds on Windws without forcing target to always be release. I did some real quick changes and got about 2x speedup, see rBe4c5441286f.

If you really debug something else, you can disable locales all together, including all other areas you don't need. That will speed up your re-compile / re-link time a lot anyway.

Alternatively, we can enable per-executable-target CFLAGS, similar to what we can do for library targets already:

P459 Per-executable cflags

1	diff --git a/build_files/cmake/macros.cmake b/build_files/cmake/macros.cmake
2	index f9992ee92b9..7e765ce649d 100644
3	--- a/build_files/cmake/macros.cmake
4	+++ b/build_files/cmake/macros.cmake
5	@@ -276,6 +276,13 @@ function(blender_add_lib
6	set_property(GLOBAL APPEND PROPERTY BLENDER_LINK_LIBS ${name})
7	endfunction()
8
9	+function(blender_add_executable
10	+ name
11	+ sources
12	+ )
13	+ add_cc_flags_custom_test(${name} PARENT_SCOPE)
14	+ add_executable("${name}" "${sources}")
15	+endfunction()
16
17	function(SETUP_LIBDIRS)
18
19	diff --git a/intern/locale/CMakeLists.txt b/intern/locale/CMakeLists.txt
20	index 6896702fcbf..1ed80990fbf 100644
21	--- a/intern/locale/CMakeLists.txt
22	+++ b/intern/locale/CMakeLists.txt
23	@@ -72,7 +72,7 @@ endif()
24	set(MSFFMT_SRC
25	msgfmt.cc
26	)
27	-add_executable(msgfmt ${MSFFMT_SRC})
28	+blender_add_executable(msgfmt ${MSFFMT_SRC})
29
30	if(CMAKE_C_COMPILER_ID MATCHES "Clang" AND (NOT (CMAKE_C_COMPILER_VERSION VERSION_LESS 3.4)))
31	# needed for clang 3.4+

This revision now requires changes to proceed.Apr 5 2017, 11:38 AM

In D2601#60906, @Sergey Sharybin (sergey) wrote:

I am not happy at all with such a tricks to force particular targets to be release. If one requests Blender to be in debug mode, it should be so. Unrelated how often one debugs the code.

I'm not a big fan either, however a with 29x speedup in runtime, (18.7x with rBe4c5441286f) and nearly 8% reduction in global build time, I'm willing to change my mind quite rapidly ,at this point i'm willing to consider who we're punishing (all windows devs) by not doing it, and who might be negatively impacted by these changes (lets be honest, git blame reveals..... it's just you, and you're not even on this platform 99.9% of the time) , so based on that information I decided to propose this change.

The same exact code needs about 4sec on Linux in debug mode to generate all locales, so there should be a way to optimize debug builds on Windws without forcing target to always be release.

There really isn't. There's just a lot of error checking going on, on top of that std::map+std::string do tons and tons of tiny allocations making the debug heap really busy.

baseline : 116 seconds (going with msvc 2015)
rBe4c5441286f : 75 seconds

we could remove the checked iterators (basic error checking on stl)

#define _ITERATOR_DEBUG_LEVEL 0 : 51.9 seconds.

we could undefine _DEBUG and get rid of some more runtime checking (this does however also means leaving behind /MTd and using the regular heap (/MT)

undefine _DEBUG /MTd : 19.9 seconds.

and the last thing we could do is optimize the code

build with /O2 : 4 seconds

At this point however we build our selves a release build...

option b, we rewrite msgfmt not to use stl::map+std::string as much, just because a single platforms debug configuration is being difficult, but that sounds like a giant waste of time....

I did some real quick changes and got about 2x speedup, see rBe4c5441286f.

although 2x is certainly impressive (great find there!), it's still 18.7x from where i want it to be at.

If you really debug something else, you can disable locales all together, including all other areas you don't need. That will speed up your re-compile / re-link time a lot anyway.

True, you can turn this off, but most especially new dev's won't do that. They just run our make.bat and go with the defaults. Then complain about the build speeds, this change is one in larger pet project of mine, taking a good look at our build times (not just debug, this one just happens to only apply to debug) and see if they are actually bad, why they are bad, and what we can do about it. D2595 is another, there's more coming, some of them will be controversial and we'll probably also have disagreements, and that's all-right.

Alternatively, we can enable per-executable-target CFLAGS, similar to what we can do for library targets already:

P459 Per-executable cflags
1 diff --git a/build_files/cmake/macros.cmake b/build_files/cmake/macros.cmake
2 index f9992ee92b9..7e765ce649d 100644
3 --- a/build_files/cmake/macros.cmake
4 +++ b/build_files/cmake/macros.cmake
5 @@ -276,6 +276,13 @@ function(blender_add_lib
6 set_property(GLOBAL APPEND PROPERTY BLENDER_LINK_LIBS ${name})
7 endfunction()
8
9 +function(blender_add_executable
10 + name
11 + sources
12 + )
13 + add_cc_flags_custom_test(${name} PARENT_SCOPE)
14 + add_executable("${name}" "${sources}")
15 +endfunction()
16
17 function(SETUP_LIBDIRS)
18
19 diff --git a/intern/locale/CMakeLists.txt b/intern/locale/CMakeLists.txt
20 index 6896702fcbf..1ed80990fbf 100644
21 --- a/intern/locale/CMakeLists.txt
22 +++ b/intern/locale/CMakeLists.txt
23 @@ -72,7 +72,7 @@ endif()
24 set(MSFFMT_SRC
25 msgfmt.cc
26 )
27 -add_executable(msgfmt ${MSFFMT_SRC})
28 +blender_add_executable(msgfmt ${MSFFMT_SRC})
29
30 if(CMAKE_C_COMPILER_ID MATCHES "Clang" AND (NOT (CMAKE_C_COMPILER_VERSION VERSION_LESS 3.4)))
31 # needed for clang 3.4+

Seems like a complicated way to get the same thing done (if you want to do it like this, fine with me, i have no strong opinion on this) and it still doesn't get rid of the _DEBUG define (cmake forces that upon us), I updated the diff with a slightly different solution for that issue that doesn't generate any extra warnings.

By default make will build release build and making a debug build will be an explicit action from developer. Now, when i request build system to give me debug build, i expect it to give me the debug build. Without any exceptions.

You can apply your own points about new developer in other way around as well. What if one needs to debug this generator (hint: it is not as feature complete as the GNU's msgfmt, has missing features and so on). And then all of a sudden he'll be struggling about his development environment not behaving correct.

Or within several month even core developers will forget about such a hack and will also waste time trying to get things properly debuggable.

I would not care if someone rewrites msgfmt in C using out BLI utilities to work with map and dynamic strings (which will solve the STL rubbish from MSVC). But under no circumstances i will accept such changes. Just period.

If other core devs are fine with this change and happy to pick up maintenance i will shut up as well.

That argument only works one way, you have all windows devs saving 2 minutes/full builds on daily basis, vs that one guy that's going to lose 1 minute going why is my debugger so whacky!? looks at the top of the file or the cmakelist to figure out what's going on and why in the rare event this code needs work (6 commits in 4 years)

last effort on compromise, what if i wrap it in an

option(WITH_WINDOWS_USE_RELEASE_MSGFMT_FOR_DEBUG "Use a release version of msgfmt to speed up building of locales in a debug build" ON)
mark_as_advanced(WITH_WINDOWS_USE_RELEASE_MSGFMT_FOR_DEBUG)

You can still get a genuine debug build if you want, and it speeds it the building of locales for the rest of us.

Having slow build times by default on Windows is a problem. Just heard from @Germano Cavalcante (mano-wii) say debug builds takes around an hour. GSOC students and new developers probably don't know which options to disable.

@Sergey Sharybin (sergey) agree the current patch isn't great, but having per-executable build flags gets a bit annoying and also isn't really convenient for MSVC users either.

Having WITH_WINDOWS_USE_RELEASE_MSGFMT_FOR_DEBUG seems too spesific.

Why not have single option for all build-helper-executable to use release flags?
People who debug makesdna/makesrna/msgfmt/datatoc... (which isn't real common) just need to disable this option.

Or only enable this by default on MSVC, with note that MSVC has unreasonably slow build times without the option.

Campbell Barton (campbellbarton) added a subscriber: Germano Cavalcante (mano-wii).Apr 6 2017, 7:00 AM

Why to expose some obscure option which is only needed by one particular generator compiled with one specific target at a specific platform to all possible generators? Blender is already tricky enough to get properly debuggable, with such flag (especially if it's enabled by default) it'll became even less trivial to debug. All of a sudden in *some* of the targets functions will start being inlined, variables will start being optimized out giving a major PITA watching variables. As additional downside, those targets will stop using debug flags, where you might have some ASAN/LSAN enabled and you'll have false idea that all sanitizers are passing there.

If one invested time which is wasted here on defence of weak solution to port msgfmt to BLI_ghash and BLI_dynamic_string he would already have done with that.

P.S. The claim that nobody debugs the code is weird on it's nature. Shall we then undefine _DEBUG from all librarikes in extern/? How far would you guys push such a solutions then?

Not claiming nobody debugs this code, just that its not an active area of development, check the commit logs of these programs - they go for months (years in some cases) without being touched,
they are simple programs that don't require much maintenance.

AFAICS this impacts most Windows developers?, so its not _that_ obscure, I don't use windows but just getting someone to test a 4 line patch took far too long (cant recall exactly was over 20min IIRC).

Agree its real crappy, but there is some tradeoff between providing a good developer environment and ability to debug build programs that are rarely touched. :S

The difference between build-exe-helpers and extern/ is these aren't included in Blender's binary, they're only used for generation.

Note, not real fussed on this patch - think its fine to close.

It could be handled some other way - put a release version of msgfmt in lib/ for eg.

oops, double post

For the record: i needed debug symbols for all generators several times over the past 5-6 months. The fact that there was no commits done in the generators does not mean their behavior was not investigated on some less commonly used platform.

If nobody wants to put effort and address root of the issue by getting away from STL, then see inlined comments about possible solution i can live with if others agree with that. Basically: always be explicit of whatever divergency from requested configuration you do.

I wouldn't put msgfmt to lib: it'll give extra work when we need to update it.

intern/locale/CMakeLists.txt
77	Make it `set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_RELEASE} /DFORCED_RELEASE_BUILD")`
intern/locale/msgfmt.cc
18	Change to #if defined( _MSC_VER ) && defined( _DEBUG) # undef _DEBUG # define FORCED_RELEASE_BUILD #endif #ifdef FORCED_RELEASE_BUILD # warning "msgfmt is forced to compile in release mode" #endif

After some profiling i'm seeing some further optimizations that might give some further gains, holding off on fighting this particular battle until i have some new numbers.

Bastien Montagne (mont29) mentioned this in D2605: Add C internal version of msgfmt (similar to makesrna/dna binaries)..Apr 7 2017, 10:52 AM

D2605 is the way to go.