From cf1670f9a6541e42f22d58e2de5940e64cb6637e Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Fri, 15 May 2020 06:50:05 +0200 Subject: Implement ad hoc recipe sharing and locking --- libbuild2/action.hxx | 7 ++ libbuild2/context.cxx | 8 ++ libbuild2/context.hxx | 10 ++- libbuild2/dist/operation.cxx | 2 +- libbuild2/module.cxx | 177 ++++++++++++++++++++++--------------------- libbuild2/parser.cxx | 70 ++++++++++------- libbuild2/parser.hxx | 4 +- libbuild2/rule.cxx | 111 +++++++++++++++++++++------ libbuild2/rule.hxx | 17 ++++- libbuild2/target.hxx | 3 + libbuild2/target.ixx | 11 ++- 11 files changed, 270 insertions(+), 150 deletions(-) diff --git a/libbuild2/action.hxx b/libbuild2/action.hxx index 5898ba8..906d7eb 100644 --- a/libbuild2/action.hxx +++ b/libbuild2/action.hxx @@ -61,6 +61,8 @@ namespace build2 { action (): inner_id (0), outer_id (0) {} // Invalid action. + action (action_id a): action (a >> 4, a & 0xF) {} + // If this is not a nested operation, then outer should be 0. // action (meta_operation_id m, operation_id inner, operation_id outer = 0) @@ -103,6 +105,11 @@ namespace build2 inline bool operator!= (action x, action y) {return !(x == y);} + inline bool operator== (action x, action_id y) {return x == action (y);} + inline bool operator!= (action x, action_id y) {return x != action (y);} + inline bool operator== (action_id x, action y) {return action (x) == y;} + inline bool operator!= (action_id x, action y) {return action (x) == y;} + bool operator> (action, action) = delete; bool operator< (action, action) = delete; bool operator>= (action, action) = delete; diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx index fe046ae..a3455ea 100644 --- a/libbuild2/context.cxx +++ b/libbuild2/context.cxx @@ -894,6 +894,14 @@ namespace build2 } phase_switch:: + phase_switch (phase_unlock&& u, phase_lock&& l) + : old_phase (u.l->phase), new_phase (l.phase) + { + phase_lock_instance = u.l; // Disarms phase_lock + u.l = nullptr; // Disarms phase_unlock + } + + phase_switch:: ~phase_switch () noexcept (false) { phase_lock* pl (phase_lock_instance); diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx index 573b8d1..8e68784 100644 --- a/libbuild2/context.hxx +++ b/libbuild2/context.hxx @@ -553,9 +553,17 @@ namespace build2 // Assuming we have a lock on the current phase, temporarily switch to a // new phase and switch back on destruction. // + // The second constructor can be used for a switch with an intermittent + // unlock: + // + // phase_unlock pu; + // phase_lock pl; + // phase_switch ps (move (pu), move (pl)); + // struct LIBBUILD2_SYMEXPORT phase_switch { - explicit phase_switch (context&, run_phase); + phase_switch (context&, run_phase); + phase_switch (phase_unlock&&, phase_lock&&); ~phase_switch () noexcept (false); run_phase old_phase, new_phase; diff --git a/libbuild2/dist/operation.cxx b/libbuild2/dist/operation.cxx index 34dc747..8dd8a6e 100644 --- a/libbuild2/dist/operation.cxx +++ b/libbuild2/dist/operation.cxx @@ -319,7 +319,7 @@ namespace build2 ctx.current_operation (op_update, nullptr, false /* diag_noise */); - action a (perform_id, update_id); + action a (perform_update_id); mo_perform.match (params, a, files, 1 /* diag (failures only) */, diff --git a/libbuild2/module.cxx b/libbuild2/module.cxx index 4972d7b..ce6c9d6 100644 --- a/libbuild2/module.cxx +++ b/libbuild2/module.cxx @@ -66,14 +66,9 @@ namespace build2 // Note: also used by ad hoc recipes thus not static. // void - create_module_context (context& ctx, const location& loc, const char* what) + create_module_context (context& ctx, const location& loc) { assert (ctx.module_context == nullptr); - - if (!ctx.module_context_storage) - fail (loc) << "unable to update " << what << - info << "updating of " << what << "s is disabled"; - assert (*ctx.module_context_storage == nullptr); // Since we are using the same scheduler, it makes sense to reuse the @@ -98,7 +93,11 @@ namespace build2 // Setup the context to perform update. In a sense we have a long-running // perform meta-operation batch (indefinite, in fact, since we never call // the meta-operation's *_post() callbacks) in which we periodically - // execute the update operation. + // execute update operations. + // + // Note that we perform each build in a separate update operation. Failed + // that, if the same target is update twice (which may happen with ad hoc + // recipes) we will see the old state. // if (mo_perform.meta_operation_pre != nullptr) mo_perform.meta_operation_pre ({} /* parameters */, loc); @@ -107,88 +106,74 @@ namespace build2 if (mo_perform.operation_pre != nullptr) mo_perform.operation_pre ({} /* parameters */, update_id); - - ctx.module_context->current_operation (op_update); } // Note: also used by ad hoc recipes thus not static. // const target& update_in_module_context (context& ctx, const scope& rs, names tgt, - const location& loc, const path& bf, - const char* what, const char* name) + const location& loc, const path& bf) { - action_targets tgs; - { - action a (perform_id, update_id); - - // Cutoff the existing diagnostics stack and push our own entry. - // - diag_frame::stack_guard diag_cutoff (nullptr); - - auto df = make_diag_frame ( - [&loc, what, name] (const diag_record& dr) - { - dr << info (loc) << "while " << what; + // New update operation. + // + ctx.module_context->current_operation (op_update); - if (name != nullptr) - dr << ' ' << name; - }); + // Un-tune the scheduler. + // + // Note that we can only do this if we are running serially because + // otherwise we cannot guarantee the scheduler is idle (we could have + // waiting threads from the outer context). This is fine for now since the + // only two tuning level we use are serial and full concurrency (turns out + // currently we don't really need this: we will always be called during + // load or match phases and we always do parallel match; but let's keep it + // in case things change). + // + auto sched_tune (ctx.sched.serial () + ? scheduler::tune_guard (ctx.sched, 0) + : scheduler::tune_guard ()); + + // Remap verbosity level 0 to 1 unless we were requested to be silent. + // Failed that, we may have long periods of seemingly nothing happening + // while we quietly update the module, which may look like things have + // hung up. + // + // @@ CTX: modifying global verbosity level won't work if we have multiple + // top-level contexts running in parallel. + // + auto verbg = make_guard ( + [z = !silent && verb == 0 ? (verb = 1, true) : false] () + { + if (z) + verb = 0; + }); + + // Note that for now we suppress progress since it would clash with the + // progress of what we are already doing (maybe in the future we can do + // save/restore but then we would need some sort of diagnostics that we + // have switched to another task). + // + action a (perform_update_id); + action_targets tgs; - // Un-tune the scheduler. - // - // Note that we can only do this if we are running serially because - // otherwise we cannot guarantee the scheduler is idle (we could have - // waiting threads from the outer context). This is fine for now since - // the only two tuning level we use are serial and full concurrency - // (turns out currently we don't really need this: we will always be - // called during load or match phases and we always do parallel match; - // but let's keep it in case things change). - // - auto sched_tune (ctx.sched.serial () - ? scheduler::tune_guard (ctx.sched, 0) - : scheduler::tune_guard ()); - - // Remap verbosity level 0 to 1 unless we were requested to be silent. - // Failed that, we may have long periods of seemingly nothing happening - // while we quietly update the module, which may look like things have - // hung up. - // - // @@ CTX: modifying global verbosity level won't work if we have - // multiple top-level contexts running in parallel. - // - auto verbg = make_guard ( - [z = !silent && verb == 0 ? (verb = 1, true) : false] () - { - if (z) - verb = 0; - }); - - // Note that for now we suppress progress since it would clash with - // the progress of what we are already doing (maybe in the future we - // can do save/restore but then we would need some sort of - // diagnostics that we have switched to another task). - // - mo_perform.search ({}, /* parameters */ - rs, /* root scope */ - rs, /* base scope */ - bf, /* buildfile */ - rs.find_target_key (tgt, loc), - loc, - tgs); - - mo_perform.match ({}, /* parameters */ - a, - tgs, - 1, /* diag (failures only) */ - false /* progress */); - - mo_perform.execute ({}, /* parameters */ - a, - tgs, - 1, /* diag (failures only) */ - false /* progress */); - } + mo_perform.search ({}, /* parameters */ + rs, /* root scope */ + rs, /* base scope */ + bf, /* buildfile */ + rs.find_target_key (tgt, loc), + loc, + tgs); + + mo_perform.match ({}, /* parameters */ + a, + tgs, + 1, /* diag (failures only) */ + false /* progress */); + + mo_perform.execute ({}, /* parameters */ + a, + tgs, + 1, /* diag (failures only) */ + false /* progress */); assert (tgs.size () == 1); return tgs[0].as (); @@ -348,7 +333,13 @@ namespace build2 // Create the build context if necessary. // if (ctx.module_context == nullptr) - create_module_context (ctx, loc, "build system module"); + { + if (!ctx.module_context_storage) + fail (loc) << "unable to update build system module " << mod << + info << "building of build system modules is disabled"; + + create_module_context (ctx, loc); + } // Inherit loaded_modules lock from the outer context. // @@ -378,17 +369,27 @@ namespace build2 } else { - const scope& rs (lr.second); + const target* l; + { + // Cutoff the existing diagnostics stack and push our own entry. + // + diag_frame::stack_guard diag_cutoff (nullptr); - const target& l ( - update_in_module_context ( - ctx, rs, move (lr.first), - loc, path (), "loading build system module", mod.c_str ())); + auto df = make_diag_frame ( + [&loc, &mod] (const diag_record& dr) + { + dr << info (loc) << "while loading build system module " << mod; + }); + + l = &update_in_module_context ( + ctx, lr.second, move (lr.first), + loc, path ()); + } - if (!l.is_a ("libs")) + if (!l->is_a ("libs")) fail (loc) << "wrong export from build system module " << mod; - lib = l.as ().path (); + lib = l->as ().path (); l5 ([&]{trace << "updated " << lib;}); } diff --git a/libbuild2/parser.cxx b/libbuild2/parser.cxx index 8d6beea..588968e 100644 --- a/libbuild2/parser.cxx +++ b/libbuild2/parser.cxx @@ -722,8 +722,12 @@ namespace build2 // that token part of the replay (we cannot peek past the replay // sequence). // - auto parse = [this, &st] (token& t, type& tt, - const target_type* type, string pat) + auto parse = [ + this, + &st, + recipes = small_vector, 1> ()] + (token& t, type& tt, + const target_type* type, string pat) mutable { token rt; // Recipe start token. @@ -752,7 +756,7 @@ namespace build2 if (type != nullptr) fail (rt) << "recipe in target type/pattern"; - parse_recipe (t, tt, rt); + parse_recipe (t, tt, rt, recipes); }; for_each (parse); @@ -1012,7 +1016,9 @@ namespace build2 } void parser:: - parse_recipe (token& t, type& tt, const token& start) + parse_recipe (token& t, type& tt, + const token& start, + small_vector, 1>& recipes) { // Parse a recipe chain. // @@ -1038,11 +1044,11 @@ namespace build2 default_target_ = target_; } - // True if seen a recipe that requires cleanup. - // - bool clean (false); + bool first (recipes.empty ()); // First target. + bool clean (false); // Seen a recipe that requires cleanup. - for (token st (start);; st = t) + token st (start); + for (size_t i (0);; st = t, ++i) { optional diag; @@ -1111,31 +1117,37 @@ namespace build2 // me now. // shared_ptr ar; + if (first) + { + // Note that this is always the location of the opening multi-curly- + // brace, whether we have the header or not. This is relied upon by + // the rule implementations (e.g., to calculate the first line of the + // recipe code). + // + location loc (get_location (st)); - // Note that this is always the location of the opening multi-curly- - // brace, whether we have the header or not. This is relied upon by the - // rule implementations (e.g., to calculate the first line of the recipe - // code). - // - location loc (get_location (st)); + if (!lang) + { + ar.reset (new adhoc_script_rule (move (t.value), + move (diag), + loc, + st.value.size ())); + } + else if (*lang == "c++") + { + ar.reset (new adhoc_cxx_rule (move (t.value), loc, st.value.size ())); + clean = true; + } + else + fail (lloc) << "unknown recipe language '" << *lang << "'"; - if (!lang) - { - ar.reset (new adhoc_script_rule (move (t.value), - move (diag), - loc, - st.value.size ())); - } - else if (*lang == "c++") - { - ar.reset (new adhoc_cxx_rule (move (t.value), loc, st.value.size ())); - clean = true; + recipes.push_back (ar); } else - fail (lloc) << "unknown recipe language '" << *lang << "'"; + ar = recipes[i]; - action a (perform_id, update_id); - target_->adhoc_recipes.push_back (adhoc_recipe {a, move (ar)}); + target_->adhoc_recipes.push_back ( + adhoc_recipe {perform_update_id, move (ar)}); next (t, tt); assert (tt == type::multi_rcbrace); @@ -1152,7 +1164,7 @@ namespace build2 // if (clean) { - action a (perform_id, clean_id); + action a (perform_clean_id); auto f (&adhoc_rule::clean_recipes_build); // First check if we have already done this. diff --git a/libbuild2/parser.hxx b/libbuild2/parser.hxx index 5b930c5..7a44e65 100644 --- a/libbuild2/parser.hxx +++ b/libbuild2/parser.hxx @@ -112,7 +112,9 @@ namespace build2 string = string ()); void - parse_recipe (token&, token_type&, const token&); + parse_recipe (token&, token_type&, + const token&, + small_vector, 1>&); // Ad hoc target names inside < ... >. // diff --git a/libbuild2/rule.cxx b/libbuild2/rule.cxx index ac45305..a4ae830 100644 --- a/libbuild2/rule.cxx +++ b/libbuild2/rule.cxx @@ -17,6 +17,13 @@ using namespace butl; namespace build2 { + // rule (vtable) + // + rule:: + ~rule () + { + } + // file_rule // // Note that this rule is special. It is the last, fallback rule. If @@ -592,6 +599,12 @@ namespace build2 // adhoc_cxx_rule // + adhoc_cxx_rule:: + ~adhoc_cxx_rule () + { + delete impl.load (memory_order_relaxed); // Serial execution. + } + void adhoc_cxx_rule:: dump (ostream& os, const string& ind) const { @@ -606,12 +619,11 @@ namespace build2 // From module.cxx. // void - create_module_context (context&, const location&, const char* what); + create_module_context (context&, const location&); const target& update_in_module_context (context&, const scope&, names tgt, - const location&, const path& bf, - const char* what, const char* name); + const location&, const path& bf); pair load_module_library (const path& lib, const string& sym, string& err); @@ -650,10 +662,21 @@ namespace build2 // string id (sha256 (code).abbreviated_string (12)); - // @@ TODO: locking. - // @@ Need to unlock phase while waiting. - if (impl == nullptr) + // We use the relaxed memory order because any change must go through the + // serial load phase. In other words, all we need here is atomicity with + // ordering/visibility provided by the phase mutex. + // + cxx_rule* impl (this->impl.load (memory_order_relaxed)); + + while (impl == nullptr) // Breakout loop. { + // Switch the phase to (serial) load and re-check. + // + phase_switch ps (ctx, run_phase::load); + + if ((impl = this->impl.load (memory_order_relaxed)) != nullptr) + break; + using create_function = cxx_rule* (const location&); using load_function = create_function* (); @@ -663,10 +686,6 @@ namespace build2 string sym ("load_" + id); - // Switch the phase to load. - // - phase_switch ps (ctx, run_phase::load); - optional altn (false); // Standard naming scheme. if (!is_src_root (pd, altn)) { @@ -734,7 +753,7 @@ namespace build2 // If we want the user to be able to supply a custom constuctor, // then we have to give the class a predictable name (i.e., we - // cannot use id as part of its name) and put it into an anonymous + // cannot use id as part of its name) and put it into an unnamed // namespace. One clever idea is to call the class `constructor` but // the name could also be used for a custom destructor (still could // work) or for name qualification (would definitely look bizarre). @@ -833,14 +852,22 @@ namespace build2 } } - const target* l; + // Update the library target in the module context. + // + const target* l (nullptr); { bool nested (ctx.module_context == &ctx); // Create the build context if necessary. // if (ctx.module_context == nullptr) - create_module_context (ctx, loc, "ad hoc recipe"); + { + if (!ctx.module_context_storage) + fail (loc) << "unable to update ad hoc recipe for target " << t << + info << "building of ad hoc recipes is disabled"; + + create_module_context (ctx, loc); + } // "Switch" to the module context. // @@ -848,26 +875,63 @@ namespace build2 // Load the project in the module context. // + // Note that it's possible it has already been loaded (see above about + // the id calculation). + // path bf (pd / std_buildfile_file); scope& rs (load_project (ctx, pd, pd, false /* forwarded */)); - source (rs, rs, bf); - if (nested) + // If the project has already been loaded then, as an optimization, + // check if the target has already been updated (this will make a + // difference we if we have identical recipes in several buildfiles). + // + if (!source_once (rs, rs, bf)) { - // @@ TODO: we probably want to make this work. + const target_type* tt (rs.find_target_type ("libs")); + assert (tt != nullptr); - fail (loc) << "nested ad hoc recipe updates not yet supported" << endf; + l = ctx.targets.find (*tt, pd, dir_path () /* out */, id); + assert (l != nullptr); + + if (l->executed_state (perform_update_id) == target_state::unknown) + l = nullptr; } - else + + if (l == nullptr) { - l = &update_in_module_context ( - ctx, rs, names {name (pd, "libs", id)}, - loc, bf, "updating ad hoc recipe", nullptr); + if (nested) + { + // @@ TODO: we probably want to make this work. + + fail (loc) << "nested ad hoc recipe updates not yet supported"; + } + else + { + // Cutoff the existing diagnostics stack and push our own entry. + // + diag_frame::stack_guard diag_cutoff (nullptr); + + auto df = make_diag_frame ( + [this, &t] (const diag_record& dr) + { + dr << info (loc) << "while updating ad hoc recipe for target " + << t; + }); + + l = &update_in_module_context ( + ctx, rs, names {name (pd, "libs", id)}, + loc, bf); + } } } + // Load the library. + // const path& lib (l->as ().path ()); + // Note again that it's possible the library has already been loaded + // (see above about the id calculation). + // string err; pair hs (load_module_library (lib, sym, err)); @@ -891,7 +955,8 @@ namespace build2 load_function* lf (function_cast (hs.second)); create_function* cf (lf ()); - impl.reset (cf (loc)); + impl = cf (loc); + this->impl.store (impl, memory_order_relaxed); // Still in load phase. } } @@ -901,6 +966,6 @@ namespace build2 recipe adhoc_cxx_rule:: apply (action a, target& t) const { - return impl->apply (a, t); + return impl.load (memory_order_relaxed)->apply (a, t); } } diff --git a/libbuild2/rule.hxx b/libbuild2/rule.hxx index ca17f2e..79747d5 100644 --- a/libbuild2/rule.hxx +++ b/libbuild2/rule.hxx @@ -22,7 +22,7 @@ namespace build2 // // Note: match() is only called once but may not be followed by apply(). // - class rule + class LIBBUILD2_SYMEXPORT rule { public: virtual bool @@ -33,6 +33,9 @@ namespace build2 rule () = default; + virtual + ~rule (); + rule (const rule&) = delete; rule& operator= (const rule&) = delete; }; @@ -216,11 +219,17 @@ namespace build2 dump (ostream&, const string&) const override; adhoc_cxx_rule (string c, const location& l, size_t b) - : adhoc_rule (l, b), code (move (c)) {} + : adhoc_rule (l, b), code (move (c)), impl (nullptr) {} + + virtual + ~adhoc_cxx_rule () override; public: - string code; - mutable unique_ptr impl; + // Note that this recipe (rule instance) can be shared between multiple + // targets which could all be matched in parallel. + // + const string code; + mutable atomic impl; }; } diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx index b237f1f..932aa22 100644 --- a/libbuild2/target.hxx +++ b/libbuild2/target.hxx @@ -583,6 +583,9 @@ namespace build2 // This function can only be called during execution if we have observed // (synchronization-wise) that this target has been executed. // + // It can also be called during the serial load phase (but make sure you + // understand what you are doing). + // target_state executed_state (action, bool fail = true) const; diff --git a/libbuild2/target.ixx b/libbuild2/target.ixx index bb30c9c..36ca3c8 100644 --- a/libbuild2/target.ixx +++ b/libbuild2/target.ixx @@ -114,8 +114,6 @@ namespace build2 inline pair target:: matched_state_impl (action a) const { - assert (ctx.phase == run_phase::match); - // Note that the "tried" state is "final". // const opstate& s (state[a]); @@ -138,13 +136,14 @@ namespace build2 inline target_state target:: executed_state_impl (action a) const { - assert (ctx.phase == run_phase::execute); return (group_state (a) ? group->state : state)[a].state; } inline target_state target:: matched_state (action a, bool fail) const { + assert (ctx.phase == run_phase::match); + // Note that the target could be being asynchronously re-matched. // pair r (matched_state_impl (a)); @@ -158,6 +157,8 @@ namespace build2 inline pair target:: try_matched_state (action a, bool fail) const { + assert (ctx.phase == run_phase::match); + pair r (matched_state_impl (a)); if (fail && r.first && r.second == target_state::failed) @@ -169,6 +170,8 @@ namespace build2 inline target_state target:: executed_state (action a, bool fail) const { + assert (ctx.phase == run_phase::execute || ctx.phase == run_phase::load); + target_state r (executed_state_impl (a)); if (fail && r == target_state::failed) @@ -193,6 +196,8 @@ namespace build2 inline bool target:: unchanged (action a) const { + assert (ctx.phase == run_phase::match); + return matched_state_impl (a).second == target_state::unchanged; } -- cgit v1.1