Repository navigation
fix: avoid exponential MSVC compile time in covariant try_get - #722
dornbirndevelops wants to merge 2 commits into
Conversation
|
@dornbirndevelops, could you please add a benchmark for the fix to the test suite? |
|
Hi @PavelGuzenfeld,
I've integrated the minimum reproducible example from the original issue in commit 5ac2fec into the test suite. |
I misunderstood. I was curious about the compile time improvement. |
5ac2fec to
5dbb6c7
Compare
PavelGuzenfeld
left a comment
There was a problem hiding this comment.
See review comments per file.
There was a problem hiding this comment.
- Test passes against
masterso reverting the fix would leave CI green. - The test doesn't cover the covariant overloads.
There was a problem hiding this comment.
You're right on both counts. That test only had exact dependencies: with the master header it passed on gcc and clang (only MSVC ran out of heap), and it never reached #8/#9.
issue_715_many_ctor_deps.cpp replaces it and now also has derived_dep_among_many_ctor_dependencies: an implementation of an abstract interface among 27 other reference dependencies, taken as const interface&. With the master header it fails to compile on every compiler, and the reproducer next to it still runs out of heap on MSVC. That file doesn't define BOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS, so the covariant path is covered with the macro off too.
dependencies.cpp adds cases that fail on master as well (exact base& vs const derived&, const derived among other const reference deps, const vs mutable derived, void* next to a derived dep, ADL, a single forward-declared dep), and test/ft/errors checks that ambiguous and private bases don't compile.
There was a problem hiding this comment.
The PR deletes the missing_ctor_parameter<T> sentinel fallback. Now it's a hard fail. Please restore it.
There was a problem hiding this comment.
Good catch, that revision did remove it: it renamed try_get(...) to try_get_impl(...) behind a wrapper that only accepted pool pointers. sml's own call sites all pass a pool, so a missing dependency still ended up at missing_ctor_parameter<T>, but try_get<T> on anything else became a hard error.
Since then the try_get(...) declaration and definition are back unchanged, and #1–#7 are untouched. The covariant lookup is one additional overload that drops out when there is no candidate, so a missing dependency still resolves to the sentinel. The one exception is intended: several dependencies derived from the requested type are now a static_assert instead of silently falling back to a default object.
5dbb6c7 to
f60d1ab
Compare
f60d1ab to
957cab7
Compare
|
Hi @PavelGuzenfeld,
The solution attempts to solve both the heap allocation limit as well as the compile time. |
a69619c to
91d5c08
Compare
…ext#715) The covariant try_get overloads boost-ext#8/boost-ext#9 (boost-ext#467) deduced D from the pool's pool_type<D&> / pool_type<const D&> bases. MSVC's deduction through N matching bases is exponential in N and runs out of heap (C1060) at ~28 reference deps. On every compiler the deduction also fails as soon as more than one base matches, so a derived dep was only found as the pool's only (const) reference dep. Replace boost-ext#8/boost-ext#9 by one try_get whose slot comes from covariant_dep: - candidates are the pool's reference deps D& / const D& with a complete D and is_base_of<T, D>, listed once per pool; - only for a class T that none of the exact overloads boost-ext#1-boost-ext#7 matches; - one candidate is read, of several the single const one, otherwise a static_assert instead of the silent missing_ctor_parameter fallback. try_get(...) and boost-ext#1-boost-ext#7 are unchanged. Behaviour changes: - a derived dep is found next to other reference deps (before: compile error, or a dangling / default object for const T& / T); - an exact dep, also a T by value, wins over a derived one; - several derived deps without a single const one, and a private or ambiguous base among other deps, are compile errors; - incomplete reference deps compile and are never candidates; - a T* with a single derived dep among other reference deps no longer compiles (before: silently null). MSVC 19.44, boost-ext#715 reproducer, C++20 /Od: 24 deps 10.4 s / 8.6 GB -> 0.26 s / 94 MB, 28 deps C1060 -> 0.29 s / 108 MB. gcc 13 and clang 18 are unchanged. Tests: issue_715_many_ctor_deps.cpp (28 deps, also with a derived dep), new cases in dependencies.cpp, errors/several_derived_deps.cpp and errors/private_base_dep.cpp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
91d5c08 to
191c554
Compare
|
In context of class template 'has_exact_dep' struct e1 {};
struct base { int val = 1; };
struct der : base { der() { val = 715; } };
struct c {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](const base& b) { std::printf("val=%d\n", b.val); } = X);
}
};
int main() {
der d;
base b;
b.val = 5;
sml::sm<c> sm{std::move(b), d};
sm.process_event(e1{});
}gcc 14 |
|
In context of class template 'unique_covariant_dep' struct e1 {};
struct iface { virtual ~iface() = default; virtual int id() const = 0; };
struct a : iface { int id() const override { return 1; } };
struct b : iface { int id() const override { return 2; } };
struct c {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](iface& f) { std::printf("%d\n", f.id()); } = X);
}
};
int main() {
a x;
const b y{};
sml::sm<c> sm{x, y};
sm.process_event(e1{});
}Fails with |
|
In context of class template 'covariant_slot' struct e1 {};
struct base { int val = 1; };
struct der;
struct c1 {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](der&, int&) {} = X);
}
};
void f1(der& d, int& i) { sml::sm<c1> sm{d, i}; }
struct der : base { der() { val = 715; } };
struct c2 {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](const base& b, int&) { std::printf("val=%d\n", b.val); } = X);
}
};
int main() {
der d;
int i = 0;
f1(d, i);
sml::sm<c2> sm{d, i};
sm.process_event(e1{});
}clang 18 |
Review findings on boost-ext#722, and similar cases found by a follow-up search: - A T held by value is no exact dep anymore. It blocked the covariant lookup, and boost-ext#1 returned a copy, which a const T& slot bound as a dangling temporary (sm{std::move(base), derived} with const base&). A derived dep wins over it again, as boost-ext#8/boost-ext#9 did where they could deduce D. The T held by value is read instead of the "several derived deps" error, and instead of a dep with T as a private or ambiguous base, except for a const T& slot, where it would dangle. - A T& slot only considers mutable D& candidates: with D1& + const D2& it reads D1 instead of failing to bind D2. const T& and T slots still read D2, as boost-ext#9 did. pool(init, ...) reads its slots through try_get_slot, whose calls are qualified against ADL. - D is a candidate when D* converts to const T* (overload resolution), not when it is a complete type with is_base_of<T, D>, which was cached per dep type: clang instantiates an sm's constexpr constructor where it is used, so a dep incomplete there was not found as a base by a later sm with the same deps, and gcc 16 warns (-Wsfinae-incomplete) when a type is defined after such a check failed, e.g. for a static sm. A private, protected or ambiguous base is still a compile error to read; a volatile dep is no candidate for a T that is not volatile. - The "several derived deps" static_assert names the requested type again (clang). MSVC 19.44, 128 reference deps x 128 lookups without an exact dep: peak memory 1.6 GB -> 1.4 GB at the same time (~5.3 s); the boost-ext#715 reproducer is unchanged. Tests: new cases in dependencies.cpp, issue_715_incomplete_deps.cpp (without BOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS, whose own is_constructible check trips gcc 16 for later-defined deps) and errors/private_base_dep_next_to_base_by_value.cpp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, confirmed. The A Added as |
|
Confirmed. It didn't compile on master either, because One consequence: if the same sm takes both Added as |
|
Confirmed, thanks. The lookup no longer tests completeness at all. A dep is a candidate when Both are in the new |
|
In context of alias template 'slot_lookup_t' struct e1 {};
struct base { int val = 1; };
struct der : base { der() { val = 715; } };
struct c {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](der&, int&, base* p) { std::printf("%s\n", p ? "set" : "null"); } = X);
}
};
int main() {
der d;
int i = 0;
sml::sm<c> sm{d, i};
sm.process_event(e1{});
}Fails with |
|
In context of function template 'try_get_slot' struct e1 {};
struct base { int val = 0; };
struct a : base { a() { val = 1; } };
struct b : base { b() { val = 2; } };
struct c {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](base& r, base v) { std::printf("%d %d\n", r.val, v.val); } = X);
}
};
int main() {
a x;
const b y{};
sml::sm<c> sm{x, y};
sm.process_event(e1{});
}Prints |
|
Dead code in context of class template 'slot_kind' struct e1 {};
struct base { int val = 5; };
struct der : base { der() { val = 715; } };
struct c {
auto operator()() noexcept {
using namespace sml;
return make_transition_table(*"idle"_s + event<e1> / [](base*& p, base* const& q, der&, int&) { std::printf("%d %d\n", p->val, q->val); } = X);
}
};
int main() {
base b;
base* bp = &b;
der d;
int i = 0;
sml::sm<c> sm{bp, d, i};
sm.process_event(e1{});
}Prints |
Problem:
sm's constructor. From about 28 dependencies on, it fails with C1060 "compiler is out of heap space".try_getoverloads#8/#9from Google's NiceMock<MyMock> dependencies fail to compile if dependency is passed as reference template parameter #467 deduceDfrom apool_type<D&>/pool_type<const D&>base of the pool. When many bases match that pattern, MSVC's deduction is exponential in their number.#8only found a derived dependency when the pool held exactly one reference dependency,#9only when it held exactly one const reference dependency. Otherwise the lookup fell back tomissing_ctor_parameter, which is a compile error forBase&, but silently a dangling default-constructed object forconst Base&.Solution:
#8/#9with a singletry_getwhose slot is computed bycovariant_dep. Nothing is deduced from the pool's bases anymore.D&/const D&whoseD*converts toconst T*, tested by overload resolution. A public, unambiguous base is a candidate. A private, protected or ambiguous base is a candidate that is a compile error to read. An incompleteDhas no known bases, so it is no candidate; nothing tests whether it is complete. The list is built once per pool.Tfor which none of the exact overloads#2–#7matches (has_exact_dep). ATheld by value (#1) does not block it. Lookups of non-class types (void*,int, ...) are left to#1–#7and the fallback, as before.#9's result for e.g.D1&+const D2&); otherwise it is astatic_assertinstead of the silent fallback. Instead of that error, and instead of reading a private or ambiguous base, aTheld by value is read by#1, except for aconst T¶meter, where that copy would dangle.pool(init, ...)reads each slot throughtry_get_slot. ABase&slot only considersD&deps: withD1&+const D2&,Base&readsD1, whileconst Base&andBasereadD2. Aconst Base&that shares theBase&slot, because the sm also takesBase&, readsD1.Behaviour changes:
Base&,const Base&orBaseworks alongside other reference dependencies. This changes the runtime behaviour of code that already compiled. Aconst Base&previously bound a dangling default-constructed temporary, aBaseby value got a default object, and withBOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPSaBase&got an owned default object. Now they get the passed object. The same holds for the lookup of the state machine class and of sub-state-machine classes. ABasepassed by value next to a derived reference dependency and other reference dependencies: master read theBaseby value; now the derived dependency is read, as master did when it was the only reference dependency.#2–#7(Base&,const Base&,Base*,const Base*) wins over a derived one. Before, partial ordering let#9win: withBase&andconst Derived&, aconst Base&got the derived object and aBase&failed to compile. ABaseheld by value does not win over a derived dependency.const Base&andBasethis applies without a single const one. ForBase&it applies to several mutable ones; const ones do not count, so e.g.D1&+D2&+const D3&is an error forBase&. WithBOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPS, master gave a sliced copy ofD3there. It is not an error whenBaseis also passed by value (except forconst Base&): that one is read, as on master. Before, the lookup fell back to a default, dangling or null object.Baseis also passed by value (except forconst Base&): then that one is read, as master did next to other reference dependencies. Before, it was only diagnosed when it was the only reference dependency.-Wsfinae-incomplete).Base*parameter with a single derived reference dependency next to other reference dependencies is a compile error. Before, it silently gotnullptr./permissivemode (the default for C++14/17) deducedDfrom the first matching base, so it silently picked the first of several derived dependencies. Now that is the error from 3, or withBasealso passed by value, that one is read. (MSVC/permissivestill accepts a conversion to an ambiguous base that is also a direct base, as master did.)D1&+const D2&, aBase¶meter bindsD1. Master gave a compile error, or withBOOST_SML_CREATE_DEFAULT_CONSTRUCTIBLE_DEPSa sliced copy ofD2.Base.Tests:
test/ft/issue_715_many_ctor_deps.cpp: the reproducer from MSVC: exponential compile time and C1060 "compiler is out of heap space" #715 (28 dependencies), plus a derived dependency among 27 others (macro off). With the master header, the first one runs out of heap on MSVC and the second one fails to compile on every compiler.test/ft/dependencies.cpp, new cases that fail on master: an exactbase&preferred over aconst derived&, a const derived dependency among other const reference dependencies, a const derived dependency next to a mutable one,void*parameters next to a derived dependency, a covariant lookup unaffected by a user's declarations found by ADL, and a forward-declared type as the only reference dependency.test/ft/errors/several_derived_deps.cpp,test/ft/errors/private_base_dep.cpp: must not compile (WILL_FAIL).test/ft/dependencies.cpp:mutable_derived_dep_not_hidden_by_const_derived_dep: fails to compile on 191c554 and on master.derived_dep_preferred_over_base_held_by_value: dangles on 191c554.derived_dep_preferred_over_base_held_by_value_among_other_ref_deps: master read the by-value base.mutable_derived_dep_bound_not_copied_next_to_const_derived_dep: with the macro, master and 191c554 used a sliced copy.try_get_slotin the dep's namespace.base_held_by_value_read_next_to_several_derived_deps,base_held_by_value_read_next_to_private_derived_depandvolatile_derived_dep_is_no_candidate.test/ft/issue_715_incomplete_deps.cpp(macro off): a dep that was incomplete where a first sm was built is found as a base later (dangles on 191c554 with clang). A static sm whose reference dep is defined later (-Wsfinae-incompleteerror on 191c554 with gcc 16-Werror).test/ft/errors/private_base_dep_next_to_base_by_value.cpp: must not compile; it dangles on 191c554.Benchmark (the #715 reproducer, compile only, C++20, debug flags:
/Od /Ob0 /RTC1 /Zi /MDd /bigobj, gcc/clang-O0 -g; peak memory is MSVC job commit or max RSS; best of 3 below 5 s; i7-13700K, 64 GB RAM):MSVC 19.44.35229 x64 (VS 2022):
gcc 13.3 and clang 18.1 never had the blow-up and are unchanged (128 deps: gcc 0.85 → 0.86 s, clang 0.79 → 0.80 s).
Lookups without an exact dependency (e.g. the state machine class itself, or parameters that are not passed) scan the pool's reference dependencies once each, so they cost O(N). With 128 reference dependencies and 9 such lookups the file compiles in 0.48 s on MSVC. 128 reference dependencies with 128 such lookups take 5.8 s / 1.7 GB, where master runs out of heap.
The review round (5dfe302) leaves the #715 reproducer unchanged on MSVC, gcc 16 and clang 23. Measured side by side with 191c554, 128 reference dependencies with 128 such lookups use 1.4 instead of 1.6 GB peak memory on MSVC, at the same time.
Verified:
-fsanitize=address,undefined: all tests pass.Issue: #715 #723
Reviewers: @kris-jusiak @PavelGuzenfeld