Use idhash as index by WilliamRoebuck · Pull Request #523 · eclipse-score/lifecycle

WilliamRoebuck · 2026-08-24T16:50:05Z

Resolves #463
As part of this, I have:

Changed DependencyGraph's value iterator to return a std::pair<const Key, T> instead of T.
Added an overload to componentOf for transition_UT

TODO

Rename GraphIndex where it occurs
Rename some parameters and local variables called index where identifier would be a better fit
Rework Transition changes. Changes are a bit hacky, maybe it should store its own map of structs rather than a map to two bitsets?
Rework componentOf override in transition UT. I'm not sure how it was working before, but I had to move it. Maybe it works with a smaller change now, but we should maybe just add an override for IComponent in the componentOf header

github-actions · 2026-08-24T16:51:09Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.7.0) and connecting to it...
INFO: Invocation ID: c3796c8f-78bc-4f55-baeb-50ae9539f01c
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (51 packages loaded, 10 targets configured)

Analyzing: target //:license-check (88 packages loaded, 10 targets configured)

Analyzing: target //:license-check (148 packages loaded, 1432 targets configured)

Analyzing: target //:license-check (156 packages loaded, 4866 targets configured)

Analyzing: target //:license-check (160 packages loaded, 9218 targets configured)

Analyzing: target //:license-check (168 packages loaded, 9417 targets configured)

Analyzing: target //:license-check (169 packages loaded, 9541 targets configured)

Analyzing: target //:license-check (173 packages loaded, 11555 targets configured)

INFO: Analyzed target //:license-check (173 packages loaded, 11555 targets configured).
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 23.476s, Critical Path: 0.24s
INFO: 16 processes: 4 disk cache hit, 12 internal.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

MaciejKaszynski · 2026-08-28T11:52:28Z

-        std::vector<GraphIndex> dependents;
+        std::vector<Key> depends_on;
+        std::vector<Key> dependents;
+        bool visited{false};


Not sure if holding temporary info as members is a good idea, it does get rid of allocations if we had to make another data structure to hold the info thought 🤔

Would be good to also add @briefs to each and make it very clear that this is temp info.

Also make sure to say that methods are not expected to reset it after use and you shall reset the values at the beginning of the method

Done in 45a58bc

MaciejKaszynski · 2026-08-28T11:53:15Z

    /// before insertion (i.e. the first node is 0, second is 1, etc.).
    template <typename... Args>
-    GraphIndex emplace(Args&&... args)
+    Key try_emplace(const Key& key, Args&&... args)


You should add docstrings what happens if the emplace fails, what is returned.
Maybe would be better to return a Result also.

Added a warning and a debug assert 45a58bc

MaciejKaszynski · 2026-08-28T11:58:30Z

@@ -90,42 +90,45 @@ class DependencyGraph
    /// reserved at construction).
    std::size_t capacity() const


add noexcept

Done in 45a58bc

MaciejKaszynski · 2026-08-28T12:00:04Z


-    T& operator[](GraphIndex index)
+    T& operator[](Key key)
    {


at() can throw here if the key isn't in the map, I think might be good to use find instead. It has the same complexity as at so there is no drawback I think.
This is for all of the other methods that use at also

I don't think the dependency graph should validate every key passed to it, the caller already does this in cases where the identifier hash may not be present. I've added some @pre comments to methods where a missing key is invalid 45a58bc

MaciejKaszynski · 2026-08-28T12:01:18Z

+    void traverse(const Key start, PerNodeFn per_node)
    {
-        visited.assign(visited.size(), false);
+        for (auto& [key, value] : nodes)


Think std::for_each would look more readable here actually.

Done in 45a58bc

MaciejKaszynski · 2026-08-28T12:16:23Z


+/// @brief Returns the IComponent reference from an interface pointer. Useful for a template class that may take a
+/// variant or a generic interface
+inline IComponent& componentOf(IComponent* node)


is this really needed? seems a bit unnecessary

This is for transition_UT, it defines its own componentOf method to override the std::variant one. I could leave this change out but I thought it made the UT easier to understand. Can be removed after #427 as well.

MaciejKaszynski · 2026-08-28T12:17:49Z

    // dependencies can only be wired up once every node exists, so collect
    // them while creating the nodes
-    std::vector<std::pair<GraphIndex, std::vector<std::string>>> pending_dependencies;
+    std::vector<std::pair<IdentifierHash, std::vector<std::string>>> pending_dependencies;


I don't think you actually need to store this at all. Should be able to make dependencies to nodes that are not yet in the graph. I think we verify that there is no empty dependence in the script so should be safe (do double check though)

We still can't setup dependencies before both sides are ready unfortunately, so this is still needed

MaciejKaszynski · 2026-08-28T12:18:27Z

-            // LM_LOG_DEBUG() << "Queued node " << task.component.get().getIndex() << " for "
+            // LM_LOG_DEBUG() << "Queued node " << task.component.get().getIdentifier() << " for "
            //                << (task.type == ComponentTaskType::kDeactivate ? "deactivation" : "activation")
            //                << " execution, jobs in progress:" << jobs_in_progress_;


Maybe just get rid of the log?

Done in c3c554d

MaciejKaszynski · 2026-08-28T12:20:09Z

  private:
    /// @brief True iff @p node is a valid index into the underlying graph, i.e. in [0, size()).
-    bool isValidNode(GraphIndex node) const
+    bool isValidNode(Key node) const


I've added an assertion that Key is trivially copyable so I don't think we should use a const value: c3c554d

MaciejKaszynski · 2026-08-28T12:23:12Z

-        /// successors. Detection of dependency readiness should be reworked to remove this.
-        std::bitset<static_cast<std::size_t>(internal::ProcessLimits::kMaxProcesses)> enqueued_set{};
+        /// @brief Information we need to maintain about graph nodes for the current transition
+        std::unordered_map<Key, NodeInfo> node_information;


mmm why go with a hash map, bitset is nice because it won't allocate

I don't think we can avoid using a map here, but it will only allocate during initialisation

MaciejKaszynski · 2026-08-28T12:39:23Z

+        // Sets up or resets our stored info for this transition
+        for (auto [key, value] : graph_)
+        {
+            state_.node_information[key] = NodeInfo{};


Might be better to just set the values rather than delet and construct a new element

Done in c3c554d

github-project-automation Bot added this to LCM - Lifecycle & Health FT Aug 24, 2026

github-project-automation Bot moved this to Backlog in LCM - Lifecycle & Health FT Aug 24, 2026

WilliamRoebuck requested a deployment to workflow-approval August 24, 2026 16:50 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/use-idhash-as-index branch from 0022556 to e340109 Compare August 28, 2026 09:31

WilliamRoebuck requested a deployment to workflow-approval August 28, 2026 09:31 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/use-idhash-as-index branch from e340109 to b5bf313 Compare August 28, 2026 09:41

WilliamRoebuck requested a deployment to workflow-approval August 28, 2026 09:41 — with GitHub Actions Waiting

WilliamRoebuck requested a deployment to workflow-approval August 28, 2026 11:05 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/use-idhash-as-index branch from c13c457 to 21cc9f8 Compare August 28, 2026 11:08

WilliamRoebuck requested a deployment to workflow-approval August 28, 2026 11:08 — with GitHub Actions Waiting

WilliamRoebuck marked this pull request as ready for review August 28, 2026 11:08

WilliamRoebuck requested review from FScholPer, MaciejKaszynski, NicolasFussberger, anmittag, antonkri, pawelrutkaq and ramceb as code owners August 28, 2026 11:08

MaciejKaszynski requested changes Aug 28, 2026

View reviewed changes

MaciejKaszynski reviewed Aug 28, 2026

View reviewed changes

WilliamRoebuck added 2 commits August 28, 2026 14:21

Make id hash hashable, template dep graph

4322f5d

Graph builds

7f67856

WilliamRoebuck added 8 commits August 28, 2026 14:21

UTs building

3ef27ea

UT fixes

09bb0a2

Rename index to key/identifier

df3c2d7

Rework transition, change iterator

1418add

Add component_of overload

7cf7ae4

Dep graph changes

45a58bc

Rename index -> identifier

0a20f06

Various improvements

c3c554d

WilliamRoebuck force-pushed the feature/use-idhash-as-index branch from 21cc9f8 to c3c554d Compare August 28, 2026 14:50

WilliamRoebuck requested a deployment to workflow-approval August 28, 2026 14:50 — with GitHub Actions Waiting

		@@ -90,42 +90,45 @@ class DependencyGraph
		/// reserved at construction).
		std::size_t capacity() const

Uh oh!

Conversation

WilliamRoebuck commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

github-actions Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WilliamRoebuck commented Aug 24, 2026 •

edited

Loading

github-actions Bot commented Aug 24, 2026 •

edited

Loading