Blender Linking Time & GTests #73268

New Issue

Bastien Montagne · 2020-01-20T16:45:47+01:00

Bastien Montagne commented

2020-01-20 16:45:47 +01:00

Currently, doing a full debug build with ASAN and all GTests has become a real pain, as (on my machine, which does have 16GB RAM and a SSD), it can take over 10 minutes to link the four executables (Blender itself, and the three gtests that are also linking most of Blender's library).

While adding more RAM is always an answer (although not always possible in practice), I think we should stop making such 'need everything' tests as independent binaries.

I can see at least two ways to avoid such thing:

Do what we already do a lot: use python API and simple python scripts.
Integrate those tests that absolutely require to be written in C as part of Blender build itself (behind the right #ifdef of course).

I would expect that #1 would be enough, maybe with a little bit of #2 in the form of having some part of the RNA/Py API hidden behind the GTEST define, to only actually expose them when needed (if we really need C code for performance and/or low-level access reasons).

Currently, doing a full debug build with ASAN and all GTests has become a real pain, as (on my machine, which does have 16GB RAM and a SSD), it can take over 10 minutes to link the four executables (Blender itself, and the three gtests that are also linking most of Blender's library). While adding more RAM is always an answer (although not always possible in practice), I think we should stop making such 'need everything' tests as independent binaries. I can see at least two ways to avoid such thing: - Do what we already do a lot: use python API and simple python scripts. - Integrate those tests that absolutely require to be written in C as part of Blender build itself (behind the right #ifdef of course). I would expect that #1 would be enough, maybe with a little bit of #2 in the form of having some part of the RNA/Py API hidden behind the GTEST define, to only actually expose them when needed (if we really need C code for performance and/or low-level access reasons).

Bastien Montagne commented

2020-01-20 16:45:47 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Bastien Montagne commented

2020-01-20 16:45:47 +01:00

Added subscribers: @mont29, @brecht, @Sergey, @ideasman42, @dfelinto, @dr.sybren

Howard Trickey commented

2020-01-20 21:11:33 +01:00

Added subscriber: @howardt

Howard Trickey commented

2020-01-20 21:11:33 +01:00

I think I'm responsible for starting this, about 6 years ago, with bmesh_core_test.cc. I was ambitious to start unit testing all of the primitives used inside BMesh. Many of those functions are not exposed in the Python API, and probably shouldn't be. The tendrils of those primitives extend so far into other parts of Blender that it seemed very hard to isolate small libraries to link against that would let those bmesh files compile & link.

But I never added any more than the trivial proof-of-concept test case that is in that file, and nobody else stepped up to do it either. And honestly, that code works well and rarely (if ever?) breaks, so I would be OK with just deleting that test.

I don't know about the other two gtests referenced in this bug that link against all of Blender.

I think I'm responsible for starting this, about 6 years ago, with bmesh_core_test.cc. I was ambitious to start unit testing all of the primitives used inside BMesh. Many of those functions are not exposed in the Python API, and probably shouldn't be. The tendrils of those primitives extend so far into other parts of Blender that it seemed very hard to isolate small libraries to link against that would let those bmesh files compile & link. But I never added any more than the trivial proof-of-concept test case that is in that file, and nobody else stepped up to do it either. And honestly, that code works well and rarely (if ever?) breaks, so I would be OK with just deleting that test. I don't know about the other two gtests referenced in this bug that link against all of Blender.

Sybren A. Stüvel commented

2020-01-21 11:43:09 +01:00

For the USD exporter tests I want to be able to construct a scene and inspect the result of the AbstractHierarchyIterator class. This scene construction is currently done by loading a blend file, which of course requires the majority of Blender to be there. What I think would be a good approach is to have a more 'mockable' structure in Blender. For my purpose, it would already be enough to have a dependency graph that produces objects (with their evaluated data) for the USD exporter to consume. If we could have a 'mock depsgraph' that produces test data without having to link to all of Blender, that would certainly be an improvement.

For integration tests we can use Blender + Python just fine. This would allow me to do a file load, export to USD, import into another scene, and compare the result. This, however, is not a unit test.

For the USD exporter tests I want to be able to construct a scene and inspect the result of the `AbstractHierarchyIterator` class. This scene construction is currently done by loading a blend file, which of course requires the majority of Blender to be there. What I think would be a good approach is to have a more 'mockable' structure in Blender. For my purpose, it would already be enough to have a dependency graph that produces objects (with their evaluated data) for the USD exporter to consume. If we could have a 'mock depsgraph' that produces test data without having to link to all of Blender, that would certainly be an improvement. For integration tests we can use Blender + Python just fine. This would allow me to do a file load, export to USD, import into another scene, and compare the result. This, however, is not a unit test.

Bastien Montagne commented

2020-01-21 12:35:46 +01:00

@dr.sybren I think you could add C-required unittests as part of Blender build itself, expose the main test function as a python call, and use python script to do the work? C-part of tests being only actualy built when gtests are enabled, of course.

Note that this is only a very rough random idea, we could do that in many different ways/code layout, how to do it best requires some thinking.

My point being, we cannot afford to add several minutes of linking time for each 'need whole blender' tests, so why not just put those directly as part of Blender itself?

Trying to make Blender more “mockable” might be a nice goal ultimately but I see this as a much more involved project, and we'll still end up with cases where we'll need to link most of blender parts anyway…

@dr.sybren I think you could add C-required unittests as part of Blender build itself, expose the main test function as a python call, and use python script to do the work? C-part of tests being only actualy built when gtests are enabled, of course. Note that this is only a very rough random idea, we could do that in many different ways/code layout, how to do it best requires some thinking. My point being, we cannot afford to add several minutes of linking time for each 'need whole blender' tests, so why not just put those directly as part of Blender itself? Trying to make Blender more “mockable” might be a nice goal ultimately but I see this as a much more involved project, and we'll still end up with cases where we'll need to link most of blender parts anyway…

Brecht Van Lommel commented

2020-01-21 13:01:19 +01:00

I agree this is a problem we need to solve.

Using Python where possible is fine, though if it requires adding RNA/Python API functions I'd rather just have the test written in C. Writing and maintaining tests should be as simple as possible.

An alternative would be to only compile one test binary instead of multiple, though including it as part of the Blender executable seems fine if there's a good way to do that.

I agree this is a problem we need to solve. Using Python where possible is fine, though if it requires adding RNA/Python API functions I'd rather just have the test written in C. Writing and maintaining tests should be as simple as possible. An alternative would be to only compile one test binary instead of multiple, though including it as part of the Blender executable seems fine if there's a good way to do that.

Sergey Sharybin commented

2020-01-21 14:36:02 +01:00

I don't see how's GTest at fault here. Any testing framework will have same issues when you ask to link the entire project for standalone binary. Are we specifically optimizing tests which are linking against all libraries?

To me it seems there two separate issues here:

Compilation of bmesh_test should be as fast as possible, so one can easily iterate over changes in bmesh itself. After D6642 it should be possible to link bmesh_test against bf_bmesh and not pull an entire Blender. If, for some reason, that would still pull too much, i think we have bigger architectural problem.
Iteration over changes in Blender itself should be fast, so you can more easily experiment with things like interface layout or drawing (or anything else where "think ahead - compile once" doesn't really work and you have to experiment a lot). In this case i don't know why you would want to link any other binary. It's not like you'll be running them anyway (and running testsuit could be slower than linking anyway). Not sure this is something to be addressed from our build system side, or just tell people use make blender instead of make.

I don't see how's GTest at fault here. Any testing framework will have same issues when you ask to link the entire project for standalone binary. Are we specifically optimizing tests which are linking against all libraries? To me it seems there two separate issues here: 1. Compilation of `bmesh_test` should be as fast as possible, so one can easily iterate over changes in bmesh itself. After [D6642](https://archive.blender.org/developer/D6642) it should be possible to link `bmesh_test` against `bf_bmesh` and not pull an entire Blender. If, for some reason, that would still pull too much, i think we have bigger architectural problem. 2. Iteration over changes in Blender itself should be fast, so you can more easily experiment with things like interface layout or drawing (or anything else where "think ahead - compile once" doesn't really work and you have to experiment a lot). In this case i don't know why you would want to link any other binary. It's not like you'll be running them anyway (and running testsuit could be slower than linking anyway). Not sure this is something to be addressed from our build system side, or just tell people use `make blender` instead of `make`.

Sybren A. Stüvel commented

2020-01-21 15:34:37 +01:00

In #73268#854740, @mont29 wrote:
@dr.sybren I think you could add C-required unittests as part of Blender build itself, expose the main test function as a python call, and use python script to do the work? C-part of tests being only actualy built when gtests are enabled, of course.

That doesn't sound unreasonable at all.

Trying to make Blender more “mockable” might be a nice goal ultimately but I see this as a much more involved project, and we'll still end up with cases where we'll need to link most of blender parts anyway…

Yes, this is more of a mindset for future designs than something we can just quickly do.

> In #73268#854740, @mont29 wrote: > @dr.sybren I think you could add C-required unittests as part of Blender build itself, expose the main test function as a python call, and use python script to do the work? C-part of tests being only actualy built when gtests are enabled, of course. That doesn't sound unreasonable at all. > Trying to make Blender more “mockable” might be a nice goal ultimately but I see this as a much more involved project, and we'll still end up with cases where we'll need to link most of blender parts anyway… Yes, this is more of a mindset for future designs than something we can just quickly do.

Dalai Felinto commented

2020-01-21 16:46:06 +01:00

The #test project was a playground to test subprojects and milestones, it is not a real project :)

Bastien Montagne commented

2020-01-21 21:53:49 +01:00

@Sergey I never said GTests was at fault, indeed any testing framework would give the same 'issue'. Fault here is making tests that require linking most of Blender, we simply cannot afford to have to link potentially tens of binaries like that in the future, with the three current 'all-blender-linked' tests it’s already a serious pain for me to build. Sure make blender and such ease the pain a bit, but those are lacking install, and in general we should not have to use such band-aids.

@brecht indeed a single test binary could be a good fallback, if we cannot get a satisfying 'all in blender' solution. Will try to investigate this a bit more on practical side of things in near future, and come back with some actual proposal…

@Sergey I never said GTests was at fault, indeed any testing framework would give the same 'issue'. Fault here is making tests that require linking most of Blender, we simply cannot afford to have to link potentially tens of binaries like that in the future, with the three current 'all-blender-linked' tests it’s already a serious pain for me to build. Sure `make blender` and such ease the pain a bit, but those are lacking install, and in general we should not have to use such band-aids. @brecht indeed a single test binary could be a good fallback, if we cannot get a satisfying 'all in blender' solution. Will try to investigate this a bit more on practical side of things in near future, and come back with some actual proposal…

Sergey Sharybin commented

2020-01-22 09:50:57 +01:00

I never said GTests was at fault

That's how first sentence of the task reads though :\

Sure make blender and such ease the pain a bit, but those are lacking install, and in general we should not have to use such band-aids.

I don't see make blender being a band-aid here. Why to do any extra work on linking something you don't currently need anyway? Can't become faster than that.

indeed a single test binary could be a good fallback

I don't agree here. Tests themselves should be easy/fast to iterate over. I wouldn't want to wait alembic symbols to be resolved when working on blenloader, for example.

> I never said GTests was at fault That's how first sentence of the task reads though :\ > Sure make blender and such ease the pain a bit, but those are lacking install, and in general we should not have to use such band-aids. I don't see `make blender` being a band-aid here. Why to do any extra work on linking something you don't currently need anyway? Can't become faster than that. > indeed a single test binary could be a good fallback I don't agree here. Tests themselves should be easy/fast to iterate over. I wouldn't want to wait alembic symbols to be resolved when working on blenloader, for example.

Brecht Van Lommel commented

2020-01-22 10:45:23 +01:00

In #73268#854888, @Sergey wrote:
2. Iteration over changes in Blender itself should be fast, so you can more easily experiment with things like interface layout or drawing (or anything else where "think ahead - compile once" doesn't really work and you have to experiment a lot). In this case i don't know why you would want to link any other binary. It's not like you'll be running them anyway (and running testsuit could be slower than linking anyway). Not sure this is something to be addressed from our build system side, or just tell people use make blender instead of make.

When I'm working on a core part of Blender, for me iteration means compiling and running all tests every 10 minutes. Same when I'm committing changes or reviewing patches. So for my workflow at least, this task is about making iteration that as fast as possible.

> In #73268#854888, @Sergey wrote: > 2. Iteration over changes in Blender itself should be fast, so you can more easily experiment with things like interface layout or drawing (or anything else where "think ahead - compile once" doesn't really work and you have to experiment a lot). In this case i don't know why you would want to link any other binary. It's not like you'll be running them anyway (and running testsuit could be slower than linking anyway). Not sure this is something to be addressed from our build system side, or just tell people use `make blender` instead of `make`. When I'm working on a core part of Blender, for me iteration means compiling and running all tests every 10 minutes. Same when I'm committing changes or reviewing patches. So for my workflow at least, this task is about making iteration that as fast as possible.

Sergey Sharybin commented

2020-01-22 12:11:32 +01:00

Removed subscriber: @Sergey

Sergey Sharybin commented

2020-01-22 12:11:32 +01:00

This is different workflow from what i used and what i would like to be used more.
But since i only do janitory work of fixing wrong usage of cmake you guys would come to agreement faster without me.

This is different workflow from what i used and what i would like to be used more. But since i only do janitory work of fixing wrong usage of cmake you guys would come to agreement faster without me.

Brecht Van Lommel commented

2020-05-02 17:15:49 +02:00

This problem is getting worse now with more testing being added, armature and f-curve tests specifically. I would really like to see a solution here. Regardless of what the right workflow may be, each new test adding more than a GB to the build folder with debug builds does not scale.

So I really think we should build the tests that link Blender libraries as part of the Blender binary. @mont29 did you make any progress on this?

This problem is getting worse now with more testing being added, armature and f-curve tests specifically. I would really like to see a solution here. Regardless of what the right workflow may be, each new test adding more than a GB to the build folder with debug builds does not scale. So I really think we should build the tests that link Blender libraries as part of the Blender binary. @mont29 did you make any progress on this?

Jacques Lucke commented

2020-05-04 10:54:59 +02:00

Added subscriber: @JacquesLucke

Jeroen Bakker commented

2020-05-04 11:07:22 +02:00

Added subscriber: @Jeroen-Bakker

Evan Wilson commented

2020-05-04 11:08:44 +02:00

Added subscriber: @EAW

Sybren A. Stüvel commented

2020-05-04 17:44:51 +02:00

Here are some design considerations to discuss & agree upon before we move to the technical side of things. I'll consider tests written in C and C++ as equal for this task (won't keep repeating C/C++).

Scope of This Task

Currently there is a separation between the tests in C (separate executable, their own CLI arguments) and Python. Each test is run by ctest, which handles parallelisation, test selection etc. Do we want to be able to run everything from Blender (i.e. replace ctest as front-end as well)? Personally I would keep this task simple & only address the concrete issues (too many big binaries, slow build).

Running & Selecting Tests

Once the tests are integrated with Blender itself, we should be able to select which tests to run. For this I see a few options:

Add CLI options to select & run tests. The way I envision this is that all remaining options are then interpreted by the GTest test runner, and not by Blender.
Expose tests + a way to run them to Python. The --python-expr CLI argument can then be used if selection from the CLI is required, or we could expose them in such a way that they are considered subclasses of unittest.UnitTest and discoverable by the Python unit test runner.

My preference would be the former, as it makes it easier to discover the correct CLI arguments (we could extend the --help output when WITH_GTESTS=ON). Integrating with CMake/CTest will also be a bit easier, as it reduces one level of quotes, and I don't see a reason to move away from CTests to Python's unittest runner.

We should also decide on how often we want to restart Blender during the tests. We could run it for every test case, but that would likely be slow & inconvenient. I think it's best to have one invocation for each test suite, in the same way that we now have an invocation for each test executable.

Location of Tests

Currently the tests are placed outside the source directory. We could technically turn this into its own module, but that's probably going to be quite big, and require re-linking of all the tests (including potentially large tests) when a small thing changes. We could make every directory in tests/gtests their own module, so they can be built & linked separately. Alternatively, we could move the tests into their respective modules in source, so for example tests/gtests/blenkernel would then move to source/blender/blenkernel/tests.

Personally I'm in favour of the latter. It's akin to what Golang is doing: tests are sitting next to the file they're testing, and thefile.go is tested by thefile_tests.go. This has special compiler support though, and I wouldn't want to go that far with Blender. However, I found it to be rather comfortable to have the actual code and the test for that code sitting in relatively close proximity.

Here are some design considerations to discuss & agree upon before we move to the technical side of things. I'll consider tests written in C and C++ as equal for this task (won't keep repeating C/C++). ### Scope of This Task Currently there is a separation between the tests in C (separate executable, their own CLI arguments) and Python. Each test is run by `ctest`, which handles parallelisation, test selection etc. Do we want to be able to run everything from Blender (i.e. replace ctest as front-end as well)? Personally I would keep this task simple & only address the concrete issues (too many big binaries, slow build). ### Running & Selecting Tests Once the tests are integrated with Blender itself, we should be able to select which tests to run. For this I see a few options: - Add CLI options to select & run tests. The way I envision this is that all remaining options are then interpreted by the GTest test runner, and not by Blender. - Expose tests + a way to run them to Python. The `--python-expr` CLI argument can then be used if selection from the CLI is required, or we could expose them in such a way that they are considered subclasses of `unittest.UnitTest` and discoverable by the Python unit test runner. My preference would be the former, as it makes it easier to discover the correct CLI arguments (we could extend the `--help` output when `WITH_GTESTS=ON`). Integrating with CMake/CTest will also be a bit easier, as it reduces one level of quotes, and I don't see a reason to move away from CTests to Python's unittest runner. We should also decide on how often we want to restart Blender during the tests. We could run it for every test case, but that would likely be slow & inconvenient. I think it's best to have one invocation for each test suite, in the same way that we now have an invocation for each test executable. ### Location of Tests Currently the tests are placed outside the `source` directory. We could technically turn this into its own module, but that's probably going to be quite big, and require re-linking of all the tests (including potentially large tests) when a small thing changes. We could make every directory in `tests/gtests` their own module, so they can be built & linked separately. Alternatively, we could move the tests into their respective modules in `source`, so for example `tests/gtests/blenkernel` would then move to `source/blender/blenkernel/tests`. Personally I'm in favour of the latter. It's akin to what Golang is doing: tests are sitting next to the file they're testing, and `thefile.go` is tested by `thefile_tests.go`. This has special compiler support though, and I wouldn't want to go that far with Blender. However, I found it to be rather comfortable to have the actual code and the test for that code sitting in relatively close proximity.

Brecht Van Lommel commented

2020-05-04 18:36:21 +02:00

Agreed with @dr.sybren on all points.

GTest supports parsing arguments --gtest_list_tests, --gtest_filter, etc:
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#running-test-programs-advanced-options

So perhaps we could just rely on that.

Agreed with @dr.sybren on all points. GTest supports parsing arguments `--gtest_list_tests`, `--gtest_filter`, etc: https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#running-test-programs-advanced-options So perhaps we could just rely on that.

Ray molenkamp commented

2020-05-05 03:09:01 +02:00

Added subscriber: @LazyDodo

Ray molenkamp commented

2020-05-05 03:09:01 +02:00

Funny question? how did we jump to "lets put the test inside blender!" ? I feel there may have been some internal discussion I may have missed?

Why was the idea of a single test runner binary rejected? ie I see no reason we cannot stick a bunch of tests into a single hosting binary (one that is not blender...) I don't mind a long link time once (or twice if you want to count the linking of blender itself),however 8x in parallel each munching up 7-8 gigs of ram each is a different story, twice however seems perfectly fine...

kinda feels like, putting it all in the blender binary is taking it a bit too far? and way more hassle than needed?

Funny question? how did we jump to "lets put the test inside blender!" ? I feel there may have been some internal discussion I may have missed? Why was the idea of a single test runner binary rejected? ie I see no reason we cannot stick a bunch of tests into a single hosting binary (one that is not blender...) I don't mind a long link time once (or twice if you want to count the linking of blender itself),however 8x in parallel each munching up 7-8 gigs of ram each is a different story, twice however seems perfectly fine... kinda feels like, putting it all in the blender binary is taking it a bit *too* far? and way more hassle than needed?

Bastien Montagne commented

2020-05-05 07:08:51 +02:00

Also agree with @dr.sybren essentially.

@LazyDodo I do not see why that would be too far? Extra overhead (size) in final binary should be very small compared to Blender size these days, and even two linking tasks eating gigs of ram are already an issue on e.g. almost any laptop sold currently (since getting anything beyond 16GB remains a challenge)… And twice as slow is not fine, when you have to build and test repetitively. ;)

Also agree with @dr.sybren essentially. @LazyDodo I do not see why that would be too far? Extra overhead (size) in final binary should be very small compared to Blender size these days, and even two linking tasks eating gigs of ram are already an issue on e.g. almost any laptop sold currently (since getting anything beyond 16GB remains a challenge)… And twice as slow is not fine, when you have to build and test repetitively. ;)

Sybren A. Stüvel commented

2020-05-07 12:48:02 +02:00

I was thinking of including the tests in the same compilation unit as the main code (so libbf_blenkernel.a for example). However, for building the tests we suppress a lot of warnings via the remove_strict_flags() CMake macro. Without this, we get lots of warnings like these:

In file included from /home/sybren/workspace/blender-git/blender/tests/gtests/testing/testing.h:8:0,
                 from /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/tests/BKE_fcurve_test.cc:18:
/home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1955:6: warning: "GTEST_DONT_DEFINE_ASSERT_EQ" is not defined, evaluates to 0 [-Wundef]
 #if !GTEST_DONT_DEFINE_ASSERT_EQ
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1959:6: warning: "GTEST_DONT_DEFINE_ASSERT_NE" is not defined, evaluates to 0 [-Wundef]
 #if !GTEST_DONT_DEFINE_ASSERT_NE
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1963:6: warning: "GTEST_DONT_DEFINE_ASSERT_LE" is not defined, evaluates to 0 [-Wundef]
 #if !GTEST_DONT_DEFINE_ASSERT_LE
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

These are not warnings I would like to suppress for the main Blender source, though, so I would suggest putting the tests in their own compilation unit.

I was thinking of including the tests in the same compilation unit as the main code (so `libbf_blenkernel.a` for example). However, for building the tests we suppress a lot of warnings via the `remove_strict_flags()` CMake macro. Without this, we get lots of warnings like these: ``` In file included from /home/sybren/workspace/blender-git/blender/tests/gtests/testing/testing.h:8:0, from /home/sybren/workspace/blender-git/blender/source/blender/blenkernel/tests/BKE_fcurve_test.cc:18: /home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1955:6: warning: "GTEST_DONT_DEFINE_ASSERT_EQ" is not defined, evaluates to 0 [-Wundef] #if !GTEST_DONT_DEFINE_ASSERT_EQ ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1959:6: warning: "GTEST_DONT_DEFINE_ASSERT_NE" is not defined, evaluates to 0 [-Wundef] #if !GTEST_DONT_DEFINE_ASSERT_NE ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sybren/workspace/blender-git/blender/extern/gtest/include/gtest/gtest.h:1963:6: warning: "GTEST_DONT_DEFINE_ASSERT_LE" is not defined, evaluates to 0 [-Wundef] #if !GTEST_DONT_DEFINE_ASSERT_LE ^~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` These are not warnings I would like to suppress for the main Blender source, though, so I would suggest putting the tests in their own compilation unit.

Campbell Barton commented

2020-05-07 14:30:46 +02:00

Why is it necessary to put the tests along-side the regular source files?
(in the source code).

This has the down-side that tests will show up when searching the code-base (something that's very common and done often the time during development),

can't changes to linking to done without moving the tests into source/ ?

Why is it necessary to put the tests along-side the regular source files? (in the source code). This has the down-side that tests will show up when searching the code-base (something that's very common and done often the time during development), can't changes to linking to done without moving the tests into `source/` ?

Sybren A. Stüvel commented

2020-05-07 14:36:24 +02:00

In #73268#926062, @ideasman42 wrote:
Why is it necessary to put the tests along-side the regular source files?

It is not necessary, as anything can be worked around and modified. In my experience it's nice to have tests & regular code setting next to each other. It makes the tests more visible and invites people to write more tests.

From a technical point of view, it helps with the CMake configuration. By having the tests in a subdirectory of the regular source, it inherits all the options for valid include paths, etc. This reduces the need to sync settings between the two.

> In #73268#926062, @ideasman42 wrote: > Why is it necessary to put the tests along-side the regular source files? It is not necessary, as anything can be worked around and modified. In my experience it's nice to have tests & regular code setting next to each other. It makes the tests more visible and invites people to write more tests. From a technical point of view, it helps with the CMake configuration. By having the tests in a subdirectory of the regular source, it inherits all the options for valid include paths, etc. This reduces the need to sync settings between the two.

Campbell Barton commented

2020-05-07 14:41:19 +02:00

Was it considered to build a blender library (with all symbols made public) and link tests against this?

While it's some duplication, it's only done once, instead of including much of blender in each binary.

Re-linking Blender is fairly slow these days, making an edit-build-test cycle slow if tests are running from Blender.

Was it considered to build a blender library (with all symbols made public) and link tests against this? While it's some duplication, it's only done once, instead of including much of blender in each binary. Re-linking Blender is fairly slow these days, making an edit-build-test cycle slow if tests are running from Blender.

Sybren A. Stüvel commented

2020-05-07 14:44:41 +02:00

That wouldn't matter much if we use static linking, then each test executable would still be huge.

Brecht Van Lommel commented

2020-05-07 19:22:16 +02:00

I assume Campbell is talking about dynamic linking? But we have problems with symbol conflicts and I wanted to hide more symbols (see #76442).

I would put all tests that need Blender libraries in a single statically linked binary separate from the Blender binary to solve the most urgent problem. Shipping tests in release builds is not something we should do I think, I hadn't considered that earlier.

Moving to shared libraries I think is fine to improve Blender link time as a whole, especially if we can split things up into smaller libraries. But I expect that will open a can of worms and will be a bigger project?

I assume Campbell is talking about dynamic linking? But we have problems with symbol conflicts and I wanted to hide more symbols (see #76442). I would put all tests that need Blender libraries in a single statically linked binary separate from the Blender binary to solve the most urgent problem. Shipping tests in release builds is not something we should do I think, I hadn't considered that earlier. Moving to shared libraries I think is fine to improve Blender link time as a whole, especially if we can split things up into smaller libraries. But I expect that will open a can of worms and will be a bigger project?

Ray molenkamp commented

2020-05-07 19:35:12 +02:00

Looked into that previously, to make that work it'll be a much much bigger project

Circular dependencies between the libs are going to be problematic (ie i can't link A to B if B doesn't exist yet, but i can't build B without having A) and we have quite a few of those
Exports, MSVC doesn't export anything out of a shared lib, we'll have to either attribute every function we'd like to export or resort to the cmake WINDOWS_EXPORT_ALL_SYMBOLS option, which i have to admit i have never tried on a project the size of blender.
Our dependencies between libs are... yeah... here's the deps in just the lite configuration from about 2 years ago. (based on parsing out the imports/exports from the object files, i do not trust us to have properly modeled the library inter-dependencies in cmake)

{F8518716, width=75%}

Looked into that previously, to make that work it'll be a much much bigger project - Circular dependencies between the libs are going to be problematic (ie i can't link A to B if B doesn't exist yet, but i can't build B without having A) and we have quite a few of those - Exports, MSVC doesn't export anything out of a shared lib, we'll have to either attribute every function we'd like to export or resort to the cmake `WINDOWS_EXPORT_ALL_SYMBOLS` option, which i have to admit i have never tried on a project the size of blender. - Our dependencies between libs are... yeah... here's the deps in just the lite configuration from about 2 years ago. (based on parsing out the imports/exports from the object files, i do not trust us to have properly modeled the library inter-dependencies in cmake) {[F8518716](https://archive.blender.org/developer/F8518716/image.png), width=75%}

Ray molenkamp commented

2020-05-07 20:04:20 +02:00

~~I think some reductions in link time can be had by moving some of the deps to shared libs, USD and LLVM would be on top of my list there, we've been statically linking since before i started, so i can't say i have much of a historic perspective on why we're doing that? ~~ Disabling USD+OSL+LLVM made no difference in link time so moving them to dynamic is unlikely to offer any relief in the area of link times.

DamD commented

2020-05-08 21:14:55 +02:00

Added subscriber: @damd

Sybren A. Stüvel commented

2020-05-19 16:09:21 +02:00

Added subscriber: @Sergey

Sybren A. Stüvel commented

2020-05-19 16:09:21 +02:00

I had a discussion about this with @Sergey yesterday, and we came to the conclusion that (for us anyway) the best place to put the tests would be right next to the files that are tested. This means you'd get something like:

source/blender/depsgraph/intern/builder/deg_builder_remove_noop.h
source/blender/depsgraph/intern/builder/deg_builder_remove_noop.cc
source/blender/depsgraph/intern/builder/deg_builder_remove_noop_test.cc

My earlier proposal to place tests in a directory next to the intern directory (so in the above example source/blender/depsgraph/tests) isn't so nice when there are subdirectories inside intern.

Another advantage of having tests in the same module as the files under test, is that they can directly use a non-public interface. Without this, we'd have to allow the test module to include from internal directories of another module.

I had a discussion about this with @Sergey yesterday, and we came to the conclusion that (for us anyway) the best place to put the tests would be right next to the files that are tested. This means you'd get something like: - source/blender/depsgraph/intern/builder/deg_builder_remove_noop.h - source/blender/depsgraph/intern/builder/deg_builder_remove_noop.cc - source/blender/depsgraph/intern/builder/deg_builder_remove_noop_test.cc My earlier proposal to place tests in a directory next to the `intern` directory (so in the above example `source/blender/depsgraph/tests`) isn't so nice when there are subdirectories inside `intern`. Another advantage of having tests in the same module as the files under test, is that they can directly use a non-public interface. Without this, we'd have to allow the test module to include from internal directories of another module.

Bastien Montagne commented

2020-05-19 16:44:57 +02:00

I don’t really mind where the test files are placed, as long as they are close enough for actual code to be "obviously" visible for anyone working there. So am also fine with having them in the form of foo_bar_test.cc next to foo_bar.cc implementation.

We may also want to allow both actually? Some intern/ directories are already nicely organized in sub-directories, while others (like BKE or RNA or modifiers) tend to turn into a giant flat storage of files, not sure keeping tests files on same level is best idea then?

I don’t really mind where the test files are placed, as long as they are close enough for actual code to be "obviously" visible for anyone working there. So am also fine with having them in the form of `foo_bar_test.cc` next to `foo_bar.cc` implementation. We *may* also want to allow both actually? Some `intern/` directories are already nicely organized in sub-directories, while others (like BKE or RNA or modifiers) tend to turn into a giant flat storage of files, not sure keeping tests files on same level is best idea then?

Sergey Sharybin commented

2020-05-19 17:30:56 +02:00

We may also want to allow both actually? Some intern/ directories are already nicely organized in sub-directories, while others (like BKE or RNA or modifiers) tend to turn into a giant flat storage of files, not sure keeping tests files on same level is best idea then?

There are some areas where having tests/ next to its intern/ might work best (compositor? bmesh?), but generally we should split those giant flat storage of files. For example, tracking, making, subdivision, multires are all to be moved out of BKE.

Also, it is actually easier to find foo_test.cc for currently opened foo.cc in smart editor even in the case of giant flat storage of files: foo_test.cc would be next to the current file in the explorer and you can just click on it. If the test is in separate directory it will be more scrolling around (sure you can use fuzzy search, but that's usually slower than simple click on something you already see on the screen).

There are some areas where "module-global" (test/ next to intern/) would work better (compositor? bmesh?) but would advice having those more of an exception. Simply from "predictability" point of view: so it's more commonly clear where to find test corresponding test.

> We *may* also want to allow both actually? Some `intern/` directories are already nicely organized in sub-directories, while others (like BKE or RNA or modifiers) tend to turn into a giant flat storage of files, not sure keeping tests files on same level is best idea then? There are some areas where having `tests/` next to its `intern/` might work best (compositor? bmesh?), but generally we should split those giant flat storage of files. For example, tracking, making, subdivision, multires are all to be moved out of BKE. Also, it is actually easier to find `foo_test.cc` for currently opened `foo.cc` in smart editor even in the case of giant flat storage of files: `foo_test.cc` would be next to the current file in the explorer and you can just click on it. If the test is in separate directory it will be more scrolling around (sure you can use fuzzy search, but that's usually slower than simple click on something you already see on the screen). There are some areas where "module-global" (`test/` next to `intern/`) would work better (compositor? bmesh?) but would advice having those more of an exception. Simply from "predictability" point of view: so it's more commonly clear where to find test corresponding test.

Howard Trickey commented

2020-05-19 18:13:15 +02:00

There is currently a separate directory, tests/gtests/blenlib with a lot of blenlib tests, each of which need only link against blenlib and very little else. Would the intention be to move those files to beside their source files too? And continue to have a way to build them without including all of blender?

Sybren A. Stüvel commented

2020-05-25 11:29:38 +02:00

In #73268#926265, @brecht wrote:
I would put all tests that need Blender libraries in a single statically linked binary separate from the Blender binary to solve the most urgent problem.

I agree. That's already a step forward from the current situation, although it doesn't make @mont29 happy. It also opens the door to adding more tests without significant impact on the build process.

In #73268#934899, @howardt wrote:
There is currently a separate directory, tests/gtests/blenlib with a lot of blenlib tests, each of which need only link against blenlib and very little else. Would the intention be to move those files to beside their source files too?

I would be in favour of that, yes. IMO it's not worth the hassle having some test files next to the files under test, and other test files in a separate directory. If we keep them in tests/gtests/blenlib there will always be the confusion of where to place new tests.

And continue to have a way to build them without including all of blender?

I appreciate the speed of testing that this gives. However, I wouldn't want to have two ways to build & run tests (i.e. as separate binary like we have now, and with the single-big-test-runner). This would mean that some tests would be in their own binary, whereas others would be part of the big test runner. That's fine by me, as long as it's clear which test ends up where, and one ctest invocation can run all of them

> In #73268#926265, @brecht wrote: > I would put all tests that need Blender libraries in a single statically linked binary separate from the Blender binary to solve the most urgent problem. I agree. That's already a step forward from the current situation, although it doesn't make @mont29 happy. It also opens the door to adding more tests without significant impact on the build process. > In #73268#934899, @howardt wrote: > There is currently a separate directory, tests/gtests/blenlib with a lot of blenlib tests, each of which need only link against blenlib and very little else. Would the intention be to move those files to beside their source files too? I would be in favour of that, yes. IMO it's not worth the hassle having some test files next to the files under test, and other test files in a separate directory. If we keep them in `tests/gtests/blenlib` there will always be the confusion of where to place new tests. > And continue to have a way to build them without including all of blender? I appreciate the speed of testing that this gives. However, I wouldn't want to have two ways to build & run tests (i.e. as separate binary like we have now, and with the single-big-test-runner). This would mean that some tests would be in their own binary, whereas others would be part of the big test runner. That's fine by me, as long as it's clear which test ends up where, and one `ctest` invocation can run all of them

Bastien Montagne commented

2020-05-26 14:55:32 +02:00

@dr.sybren if you think putting all tests in a single binary alongside Blender one is much easier/quicker to do than putting everything in Blender itself, then by all mean, please do. Everything is good to take, and linking two monsters will already be an order of magnitude better than linking eight or more!

Sybren A. Stüvel commented

2020-06-05 15:50:18 +02:00

I have updated D7649 with what was discussed above.

I have updated [D7649](https://archive.blender.org/developer/D7649) with what was discussed above.

Sybren A. Stüvel self-assigned this 2020-06-16 14:28:03 +02:00

blender-admin commented

2020-08-07 16:43:42 +02:00

This issue was referenced by 53d203dea8

This issue was referenced by 53d203dea8230da4e80f3cc61468a4e24ff6759c

Sybren A. Stüvel commented

2020-08-18 15:57:31 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Sybren A. Stüvel closed this issue

2020-08-18 15:57:31 +02:00

Sybren A. Stüvel commented

2020-08-18 15:57:31 +02:00

I'm closing this task, as remaining tests have now also been ported.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Blender Linking Time & GTests #73268

Scope of This Task

Running & Selecting Tests

Location of Tests