Conversation
icinga-probot
Bot
added
the
area/configuration
Al2Klimov
marked this pull request as draft
February 8, 2023 14:05
Member
Author
|
LoL, the one method is named @gethash. |
This comment was marked as outdated.
This comment was marked as outdated.
Member
|
What's the general idea here? You would have wanted to change |
Member
Author
|
At least DSL's |
Member
|
At first glance, it doesn't sound super bad if they would return elements in an unspecified order. Anyways, sorting on access could also be a viable option. It all depends on the places that iterate over dicts. |
julianbrost
removed their request for review
March 31, 2023 09:18
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Al2Klimov
marked this pull request as ready for review
May 25, 2023 16:05
Al2Klimov
marked this pull request as draft
August 26, 2026 10:43
Al2Klimov
changed the title
Al2Klimov
marked this pull request as ready for review
August 26, 2026 15:14
Member
Author
Done. |
Al2Klimov
changed the title
`Dictionary#keys()`, the global keys() function, ConfigWriter's text representation of a dictionary (`Dictionary#to_string()`, the debug console, "icinga2 object list", ...), the `for (k => v in dict)` DSL loop, and the object packer's consistent hashing all need a deterministic, sorted view of a dictionary's entries. They currently get one only because Dictionary happens to be backed by an ordered std::map. Route them through GetKeys()/GetItems() instead, which already sort explicitly, and drop the hand-rolled ObjectLock + manual iteration each one was doing to build that view itself.
Both use `std::map<String, ...>` in their own declarations, but only compiled because base/dictionary.hpp happened to pull in `<map>` for Dictionary's own backing storage.
Dictionary's hot-path operations (Get/Set/Contains/Remove) don't need ordering, so an unordered_map is the better fit. The consumers that do need sorted or self-consistent iteration were already taken care of. JSON is the one place this is visible: object key order was never part of the JSON contract, and JsonEncode() iterates the dictionary directly rather than through GetKeys()/GetItems(), so it's left as-is. base-json.cpp's "encode" test compared JSON output as an exact string, which depended on Dictionary iterating in (incidentally) sorted order. It now compares the parsed JSON structure instead, since key order is no longer guaranteed.
Sorting on every call is wasted work for callers that only need a stable snapshot (e.g. before removal). Add a `sorted` parameter, defaulting to false, and pass true at call sites that need deterministic order: DSL-visible iteration (`Dictionary#keys()`, keys(), `for (k => v in dict)`) and IcingaDB's diffing of old/new keys via std::set_difference, which requires sorted ranges.
Namespace's hot-path operations (Get/Set/Contains/Remove) don't need ordering, so an unordered_map is the better fit. The consumers that do need sorted or self-consistent iteration are also taken care of.
It's a pure lookup cache (find/insert/erase, never iterated), keyed by a trivially-hashable raw Type*, with no ordering requirement.
Pure lookup cache (find/insert by zone name, never iterated), keyed by String with no ordering requirement.
Pure lookup cache (find by facility name, never iterated), keyed by String with no ordering requirement.
Both are transient per-lookup snapshots/tables, keyed by Expression::Ptr and EventType, with no ordering requirement. EventsInbox::m_Filters is deliberately left as std::map: its per-instance m_Filter member holds a long-lived iterator across the object's lifetime, which unordered_map would invalidate on any insertion-triggered rehash by an unrelated EventsInbox.
Closure-capture table built by the config grammar (config_parser.yy) and consumed by VMOps to populate a Dictionary — order was never meaningful here, since Dictionary iteration order was already decoupled from insertion order earlier in this series.
Switch m_Services to unordered_map, and add a `sorted` parameter (default false) rather than always paying for a sort: the ~30 internal C++ callers only iterate/aggregate and don't care about order, while the DSL-visible get_services() function opts in with GetServices(true) since script authors can observe the order.
Make GetItems() sorting opt-in, mirroring Dictionary::GetKeys(): default to false for the many order-independent internal callers, true where order is externally observable — get_templates() and the /v1/templates API response. GetDefaultTemplates() sorts unconditionally instead: it drives the order default templates apply to a new object, and with hash order a conflicting attribute set by two templates would resolve arbitrarily instead of deterministically.
Its callers' filter-map container types are diverging (some will switch to unordered_map, DbQuery stays std::map) with no single concrete type able to satisfy all of them. Genericize over the map type instead of picking a side; only find() is used, so any map-like type works. Templates need their definition visible at each instantiation site, so the body moves from the .cpp into the header.
FilterArrayToInt() is already generic over the map type, so this just needs the container swap. NotificationFilterToString() feeds human-readable log messages, so it sorts its output explicitly now that map order is no longer alphabetical.
…red_map Both are keyed by `std::variant<Checkable*, String>`, hashable for free in C++17 since both alternatives already are. Checked every iteration site in checkable-dependency.cpp: GetDependencyGroups() and GetDependencies() only aggregate into a flat vector consumed order-independently (DB writes, state counting), and the one place that inserts while iterating does so into a different map than the one being walked, so there's no insert-while-iterating hazard.
Al2Klimov
changed the title
It's called once per object during activation via ImportDefaultTemplatesExpression, re-sorting the same unchanged per-type template list for every object of that type. Cache it, invalidating the entry on the two mutation paths (Register(), Unregister()) that actually change it.
None of these are ever iterated in an order-observable way (only .find()/.insert()/.erase(), or their sole iteration feeds a Dictionary that PackObject()/GetItems() already re-sorts before hashing), so unordered_set's faster lookup is a pure win.
These return/store sets of `intrusive_ptr<T>`, which order by pointer address, not by any meaningful key. So there was never a natural or documented order for callers to rely on.
Subscribe()/Unsubscribe() just register/unregister each type independently in m_Subscribers -- order of iteration over the subscribed types never affects the result, so unordered_set's faster lookup is a pure win.
fds is built from a directory scan, filtered by the `except` list, then each remaining descriptor is closed independently -- the order they're closed in is never observed, so there's no reason to pay for std::set's ordering.
Both drive a fixpoint retry loop that erases the current entry as soon as it successfully includes a zone directory, looping until a full pass makes no progress. Which of the still-eligible directories gets tried first in a given pass doesn't change the final set of loaded zones -- eventually resolvable ones all get resolved, and erase-during-iteration is just as safe for unordered_set as it is for std::set (only the erased element's iterator is invalidated, and we break out of the loop right after erasing).
QueryArg only had operator< (for std::map keys), so it couldn't be used as an unordered_set key. Add operator== plus a std::hash specialization (hashing via its existing string_view conversion, same basis as operator<), and a KeyHash functor for the pair -- pre-hashing each QueryArg before combining, since boost::hash_combine doesn't fall back to std::hash for arbitrary types. Relations are deleted independently of each other, so there was no order to preserve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
to allow O(1) reads for the sake of DSL, config load speed.
TODO