Allow merge groups to be assigned a name for logging.

Author: garretrieger

ift/encoder/closure_glyph_segmenter.cc

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <cstdint>
 #include <optional>
+#include <string>
 #include <vector>
 
 #include "absl/container/btree_map.h"
@@ -348,7 +349,12 @@ static StatusOr<std::vector<Segment>> ToOrderedSegments(
       remapped_full.insert(s_prime);
     }
 
-    VLOG(0) << "  Merge group " << group_index << " has " << remapped.size()
+    std::string name = std::to_string(group_index);
+    if (strategy.Name().has_value()) {
+      name = *strategy.Name();
+    }
+
+    VLOG(0) << "  Merge group " << name << " has " << remapped.size()
             << " segments.";
     group_index++;
 
@@ -396,6 +402,7 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
     const std::vector<SubsetDefinition>& subset_definitions,
     btree_map<SegmentSet, MergeStrategy> merge_groups,
     bool place_fallback_in_init) const {
+
   for (const auto& [segments, strategy] : merge_groups) {
     if (strategy.UseCosts()) {
       TRYV(CheckForDisjointCodepoints(subset_definitions, segments));
@@ -452,8 +459,13 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
 
   // ### Iteratively merge segments and incrementally reprocess affected data.
   size_t merger_index = 0;
+  std::string merger_name = std::to_string(merger_index);
+  if (mergers[merger_index].Strategy().Name().has_value()) {
+    merger_name = *mergers[merger_index].Strategy().Name();
+  }
+
   segment_index_t last_merged_segment_index = 0;
-  VLOG(0) << "Starting merge selection for merge group " << merger_index
+  VLOG(0) << "Starting merge selection for merge group " << merger_name
           << std::endl
           << "  " << mergers[merger_index].NumInscopeSegments()
           << " inscope segments, " << mergers[merger_index].NumCutoffSegments()
@@ -465,8 +477,13 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
 
     if (!merged.has_value()) {
       merger_index++;
+
       if (merger_index < mergers.size()) {
-        VLOG(0) << "Merge group finished, starting next group " << merger_index
+        std::string merger_name = std::to_string(merger_index);
+        if (mergers[merger_index].Strategy().Name().has_value()) {
+          merger_name = *mergers[merger_index].Strategy().Name();
+        }
+        VLOG(0) << "Merge group finished, starting next group " << merger_name
                 << std::endl
                 << "  " << mergers[merger_index].NumInscopeSegments()
                 << " inscope segments, "

ift/encoder/merge_strategy.h

@@ -114,6 +114,18 @@ class MergeStrategy {
   bool UseCosts() const { return use_costs_; }
   bool UsePatchMerges() const { return use_patch_merges_; }
 
+  std::optional<absl::string_view> Name() const {
+    if (name_.has_value()) {
+      return name_;
+    } else {
+      return std::nullopt;
+    }
+  }
+
+  void SetName(std::string name) {
+    name_ = name;
+  }
+
   uint32_t NetworkOverheadCost() const { return network_overhead_cost_; }
   uint32_t MinimumGroupSize() const { return min_group_size_; }
   uint32_t PatchSizeMinBytes() const { return patch_size_min_bytes_; }
@@ -191,6 +203,7 @@ class MergeStrategy {
         patch_size_max_bytes_(patch_size_max_bytes),
         probability_calculator_(nullptr) {}
 
+  std::optional<std::string> name_ = std::nullopt;
   bool use_costs_;
   uint32_t network_overhead_cost_;
   uint32_t min_group_size_;

util/segmenter_config.proto

@@ -110,29 +110,32 @@ message SegmenterConfig {
 // is processed independently. Merges will only be made between things within the same group.
 // Each merge group has it's own configuration and frequency data.
 message MergeGroup {
+  // Used to identify the group in logging.
+  string name = 1;
+
   // The set of segments that this merge group covers. May overlap with other merge groups.
   // If this is not specified and a heuristic config is used, then it will default to all segments.
   // If this is not specified and a cost config is used, then this will default to the set of
   // segments the provided frequency data covers.
-  SegmentsProto segment_ids = 1;
+  SegmentsProto segment_ids = 2;
 
   // Adds in segments provided in the 'feature_segments' mapping.
-  SegmentsProto feature_segment_ids = 2;
+  SegmentsProto feature_segment_ids = 3;
 
   // This is the group size (number of segments merged together) used in preprocess merging
   // of any segments covered by this merge group. Setting to 1 disables preprocess merging
   // of ungrouped segments.
-  uint32 preprocess_merging_group_size = 3 [default = 1];
+  uint32 preprocess_merging_group_size = 4 [default = 1];
 
   // If frequency data is available only segments with probability less than this will
   // be included in the preprocess merging phase. Setting this to 1.0 will make preprocess
   // merging apply to all segments. Has no effect if this merge group is using heuristic
   // merging.
-  double preprocess_merging_probability_threshold = 4 [default = 1.0];
+  double preprocess_merging_probability_threshold = 5 [default = 1.0];
 
   oneof config {
-    HeuristicConfiguration heuristic_config = 5;
-    CostConfiguration cost_config = 6;
+    HeuristicConfiguration heuristic_config = 6;
+    CostConfiguration cost_config = 7;
   }
 }
 

util/segmenter_config_util.cc

@@ -157,9 +157,11 @@ SegmenterConfigUtil::ProtoToMergeGroup(
         SegmentId{.feature = true, .id_value = feature_group_id}));
   }
 
+  MergeStrategy strategy = MergeStrategy::None();
+
   if (group.has_cost_config()) {
     CodepointSet covered_codepoints;
-    MergeStrategy strategy = TRY(
+    strategy = TRY(
         ProtoToStrategy(base_cost, group.cost_config(), covered_codepoints));
 
     if (group.has_segment_ids()) {
@@ -175,8 +177,6 @@ SegmenterConfigUtil::ProtoToMergeGroup(
     strategy.SetPreClosureGroupSize(group.preprocess_merging_group_size());
     strategy.SetPreClosureProbabilityThreshold(
         group.preprocess_merging_probability_threshold());
-
-    return std::make_pair(segment_indices, strategy);
   } else {
     if (group.has_segment_ids()) {
       segment_indices.union_set(MapToIndices(group.segment_ids(), id_to_index));
@@ -185,14 +185,18 @@ SegmenterConfigUtil::ProtoToMergeGroup(
       segment_indices.insert_range(0, id_to_index.size() - 1);
     }
 
-    MergeStrategy strategy =
+    strategy =
         ::util::ProtoToStrategy(base_heuristic, group.heuristic_config());
 
     strategy.SetPreClosureGroupSize(group.preprocess_merging_group_size());
     strategy.SetPreClosureProbabilityThreshold(1.0);
+  }
 
-    return std::make_pair(segment_indices, strategy);
+  if (group.has_name()) {
+    strategy.SetName(group.name());
   }
+
+  return std::make_pair(segment_indices, strategy);
 }
 
 StatusOr<btree_map<SegmentSet, MergeStrategy>>
@@ -234,6 +238,7 @@ SegmenterConfigUtil::ConfigToMergeGroups(
 
   MergeStrategy strategy = util::ProtoToStrategy(config.base_heuristic_config(),
                                                  config.ungrouped_config());
+  strategy.SetName("Ungrouped");
   strategy.SetPreClosureGroupSize(
       config.preprocess_merging_group_size_for_ungrouped());
   strategy.SetPreClosureProbabilityThreshold(1.0);