Consolidate closure statistics and long only once at the end.

garretrieger · garretrieger · commit 9f1f23b12911 · 2026-01-27T11:27:28.000-07:00
diff --git a/ift/encoder/closure_glyph_segmenter.cc b/ift/encoder/closure_glyph_segmenter.cc
@@ -437,6 +437,7 @@ StatusOr<std::vector<Merger>> ToMergers(
 static StatusOr<GlyphSegmentation> ToFinalSegmentation(
     SegmentationContext& context,
     UnmappedGlyphHandling unmapped_glyph_handling) {
+  context.LogClosureStatistics();
   return context.ToGlyphSegmentation();
 }
 
@@ -561,8 +562,6 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(
     modified.glyphs.union_set(analysis_modified_gids);
 
     TRYV(context.GroupGlyphs(modified.glyphs, modified.segments));
-
-    context.glyph_closure_cache.LogClosureCount("Condition grouping");
   }
 
   return absl::InternalError("unreachable");
@@ -594,10 +593,8 @@ ClosureGlyphSegmenter::InitializeSegmentationContext(
        segment_index++) {
     TRY(context.ReprocessSegment(segment_index));
   }
-  context.glyph_closure_cache.LogClosureCount("Inital segment analysis");
 
   TRYV(context.GroupGlyphs(context.SegmentationInfo().NonInitFontGlyphs(), {}));
-  context.glyph_closure_cache.LogClosureCount("Condition grouping");
 
   return context;
 }
diff --git a/ift/encoder/dependency_closure.cc b/ift/encoder/dependency_closure.cc
@@ -255,7 +255,7 @@ flat_hash_map<hb_codepoint_t, glyph_id_t> DependencyClosure::UnicodeToGid(
 
 StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
     const common::SegmentSet& segments, GlyphSet& and_gids, GlyphSet& or_gids,
-    GlyphSet& exclusive_gids) const {
+    GlyphSet& exclusive_gids) {
 
   // This uses a dependency graph (from harfbuzz) to infer how 'segment_id'
   // appears in the activation conditions of any glyphs reachable from it.
@@ -294,6 +294,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
     const Segment& segment = segmentation_info_->Segments().at(segment_id);
     if (!segment.Definition().feature_tags.empty()) {
       // Feature based segments not yet handled.
+      inaccurate_results_++;
       return INACCURATE;
     }
 
@@ -302,6 +303,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
 
   flat_hash_map<Node, unsigned> traversed_edge_counts;
   if (TraverseGraph(start_nodes, traversed_edge_counts) == INACCURATE) {
+    inaccurate_results_++;
     return INACCURATE;
   }
 
@@ -326,6 +328,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
   // glyphs that are reachable from any shared_glyphs found above.
   flat_hash_map<Node, unsigned> all_shared_nodes;
   if (TraverseGraph(shared_nodes, all_shared_nodes) == INACCURATE) {
+    inaccurate_results_++;
     return INACCURATE;
   }
 
@@ -339,6 +342,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(
   }
   exclusive_gids.subtract(or_gids);
 
+  accurate_results_++;
   return ACCURATE;
 }
 
diff --git a/ift/encoder/dependency_closure.h b/ift/encoder/dependency_closure.h
@@ -55,7 +55,10 @@ class DependencyClosure {
   absl::StatusOr<AnalysisAccuracy> AnalyzeSegment(const common::SegmentSet& segments,
                       common::GlyphSet& and_gids,
                       common::GlyphSet& or_gids,
-                      common::GlyphSet& exclusive_gids) const;
+                      common::GlyphSet& exclusive_gids);
+
+  uint64_t AccurateResults() const { return accurate_results_; }
+  uint64_t InaccurateResults() const { return inaccurate_results_; }
 
  private:
   enum NodeType {
@@ -191,6 +194,9 @@ class DependencyClosure {
   // These glyphs may participate in complex substitutions and as a result we can't
   // analyze via the dep graph.
   common::GlyphSet context_glyphs_;
+
+  uint64_t accurate_results_ = 0;
+  uint64_t inaccurate_results_ = 0;
 };
 
 }  // namespace ift::encoder
diff --git a/ift/encoder/glyph_closure_cache.cc b/ift/encoder/glyph_closure_cache.cc
@@ -25,8 +25,6 @@ StatusOr<common::GlyphSet> GlyphClosureCache::GlyphClosure(
   }
 
   glyph_closure_cache_miss_++;
-  closure_count_cumulative_++;
-  closure_count_delta_++;
 
   hb_subset_input_t* input = hb_subset_input_create_or_fail();
   if (!input) {
diff --git a/ift/encoder/glyph_closure_cache.h b/ift/encoder/glyph_closure_cache.h
@@ -1,7 +1,6 @@
 #ifndef IFT_ENCODER_GLYPH_CLOSURE_CACHE_H_
 #define IFT_ENCODER_GLYPH_CLOSURE_CACHE_H_
 
-#include "absl/log/log.h"
 #include "absl/status/status.h"
 #include "common/font_data.h"
 #include "common/int_set.h"
@@ -33,32 +32,17 @@ class GlyphClosureCache {
       const common::SegmentSet& segment_ids, common::GlyphSet& and_gids,
       common::GlyphSet& or_gids, common::GlyphSet& exclusive_gids);
 
-  void LogCacheStats() const {
-    double closure_hit_rate =
-        100.0 * ((double)glyph_closure_cache_hit_) /
-        ((double)(glyph_closure_cache_hit_ + glyph_closure_cache_miss_));
-    VLOG(1) << "Glyph closure cache hit rate: " << closure_hit_rate << "% ("
-            << glyph_closure_cache_hit_ << " hits, "
-            << glyph_closure_cache_miss_ << " misses)";
-  }
-
-  void LogClosureCount(absl::string_view operation) {
-    VLOG(1) << operation << ": cumulative number of glyph closures "
-            << closure_count_cumulative_ << " (+" << closure_count_delta_
-            << ")";
-    closure_count_delta_ = 0;
-  }
+  uint64_t CacheHits() const { return glyph_closure_cache_hit_; }
+  uint64_t CacheMisses() const { return glyph_closure_cache_miss_; }
 
   hb_face_t* Face() { return preprocessed_face_.get(); }
 
  private:
   common::hb_face_unique_ptr preprocessed_face_;
   common::hb_face_unique_ptr original_face_;
   absl::flat_hash_map<SubsetDefinition, common::GlyphSet> glyph_closure_cache_;
-  uint32_t glyph_closure_cache_hit_ = 0;
-  uint32_t glyph_closure_cache_miss_ = 0;
-  uint32_t closure_count_cumulative_ = 0;
-  uint32_t closure_count_delta_ = 0;
+  uint64_t glyph_closure_cache_hit_ = 0;
+  uint64_t glyph_closure_cache_miss_ = 0;
 };
 
 }  // namespace ift::encoder
diff --git a/ift/encoder/glyph_groupings.cc b/ift/encoder/glyph_groupings.cc
@@ -3,6 +3,7 @@
 #include <optional>
 
 #include "absl/container/btree_set.h"
+#include "absl/log/log.h"
 #include "common/int_set.h"
 #include "common/try.h"
 #include "ift/encoder/activation_condition.h"
diff --git a/ift/encoder/segmentation_context.cc b/ift/encoder/segmentation_context.cc
@@ -130,9 +130,6 @@ Status SegmentationContext::ReassignInitSubset(SubsetDefinition new_def) {
 
   TRYV(GroupGlyphs(changed_gids, changed_segments));
 
-  glyph_closure_cache.LogClosureCount(
-      "Segmentation reprocess for init def change.");
-
   return absl::OkStatus();
 }
 
@@ -186,6 +183,8 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids,
       PrintDiff("AND", and_gids, dep_and_gids);
       PrintDiff("OR ", or_gids, dep_or_gids);
       PrintDiff("EXC", exclusive_gids, dep_exclusive_gids);
+      LOG(ERROR) << "init codepoints = " << segmentation_info_->InitFontSegment().codepoints.ToString();
+      LOG(ERROR) << "init glyphs = " << segmentation_info_->InitFontGlyphs().ToString();
       return absl::InternalError("Depedency graph conditions does not match the closure analysis conditions");
     }
   }
diff --git a/ift/encoder/segmentation_context.h b/ift/encoder/segmentation_context.h
@@ -4,6 +4,7 @@
 #include <cstdint>
 #include <memory>
 
+#include "absl/log/log.h"
 #include "absl/status/status.h"
 #include "common/font_data.h"
 #include "common/int_set.h"
@@ -86,11 +87,29 @@ class SegmentationContext {
   absl::StatusOr<GlyphSegmentation> ToGlyphSegmentation() const {
     GlyphSegmentation segmentation =
         TRY(glyph_groupings.ToGlyphSegmentation(*segmentation_info_));
-    glyph_closure_cache.LogCacheStats();
     TRYV(ValidateSegmentation(segmentation));
     return segmentation;
   }
 
+  void LogClosureStatistics() const {
+    uint64_t dep_graph_closures = depedency_closure_->AccurateResults() * 2;
+    uint64_t potential_closures =
+      glyph_closure_cache.CacheHits() + glyph_closure_cache.CacheMisses() +
+      dep_graph_closures;
+
+    double hb_subset_rate = 100.0 * ((double) glyph_closure_cache.CacheMisses() / (double) potential_closures);
+    double cache_hit_rate = 100.0 * ((double) glyph_closure_cache.CacheHits() / (double) potential_closures);
+    double dep_graph_hit_rate = 100.0 * ((double) dep_graph_closures / (double) potential_closures);
+    uint64_t other_closures = glyph_closure_cache.CacheHits() + glyph_closure_cache.CacheMisses() - (2 * depedency_closure_->InaccurateResults());
+
+    VLOG(0) << ">> Of " << potential_closures << " potential closure operations:" << std::endl
+      << "  " << glyph_closure_cache.CacheMisses() << " (" << hb_subset_rate << "%)  were handled by hb-subset-plan" << std::endl
+      << "  " << dep_graph_closures
+      << " (" << dep_graph_hit_rate << "%) were handled by dep graph" << std::endl
+      << "  " << glyph_closure_cache.CacheHits() << " (" << cache_hit_rate << "%) were provided by the cache" << std::endl
+      << "  " << other_closures << " were from something other than AnalyzeSegment()" << std::endl;
+  }
+
   const common::SegmentSet& InertSegments() const { return inert_segments_; }
 
   const RequestedSegmentationInformation& SegmentationInfo() const {

Original file line number	Diff line number	Diff line change
`@@ -437,6 +437,7 @@ StatusOr<std::vector<Merger>> ToMergers(`
`437`	`437`	`static StatusOr<GlyphSegmentation> ToFinalSegmentation(`
`438`	`438`	`SegmentationContext& context,`
`439`	`439`	`UnmappedGlyphHandling unmapped_glyph_handling) {`
	`440`	`+ context.LogClosureStatistics();`
`440`	`441`	`return context.ToGlyphSegmentation();`
`441`	`442`	`}`
`442`	`443`
`@@ -561,8 +562,6 @@ StatusOr<GlyphSegmentation> ClosureGlyphSegmenter::CodepointToGlyphSegments(`
`561`	`562`	`modified.glyphs.union_set(analysis_modified_gids);`
`562`	`563`
`563`	`564`	`TRYV(context.GroupGlyphs(modified.glyphs, modified.segments));`
`564`		`-`
`565`		`- context.glyph_closure_cache.LogClosureCount("Condition grouping");`
`566`	`565`	`}`
`567`	`566`
`568`	`567`	`return absl::InternalError("unreachable");`
`@@ -594,10 +593,8 @@ ClosureGlyphSegmenter::InitializeSegmentationContext(`
`594`	`593`	`segment_index++) {`
`595`	`594`	`TRY(context.ReprocessSegment(segment_index));`
`596`	`595`	`}`
`597`		`- context.glyph_closure_cache.LogClosureCount("Inital segment analysis");`
`598`	`596`
`599`	`597`	`TRYV(context.GroupGlyphs(context.SegmentationInfo().NonInitFontGlyphs(), {}));`
`600`		`- context.glyph_closure_cache.LogClosureCount("Condition grouping");`
`601`	`598`
`602`	`599`	`return context;`
`603`	`600`	`}`
Original file line number	Diff line number	Diff line change
`@@ -255,7 +255,7 @@ flat_hash_map<hb_codepoint_t, glyph_id_t> DependencyClosure::UnicodeToGid(`
`255`	`255`
`256`	`256`	`StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(`
`257`	`257`	`const common::SegmentSet& segments, GlyphSet& and_gids, GlyphSet& or_gids,`
`258`		`- GlyphSet& exclusive_gids) const {`
	`258`	`+ GlyphSet& exclusive_gids) {`
`259`	`259`
`260`	`260`	`// This uses a dependency graph (from harfbuzz) to infer how 'segment_id'`
`261`	`261`	`// appears in the activation conditions of any glyphs reachable from it.`
`@@ -294,6 +294,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(`
`294`	`294`	`const Segment& segment = segmentation_info_->Segments().at(segment_id);`
`295`	`295`	`if (!segment.Definition().feature_tags.empty()) {`
`296`	`296`	`// Feature based segments not yet handled.`
	`297`	`+ inaccurate_results_++;`
`297`	`298`	`return INACCURATE;`
`298`	`299`	`}`
`299`	`300`
`@@ -302,6 +303,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(`
`302`	`303`
`303`	`304`	`flat_hash_map<Node, unsigned> traversed_edge_counts;`
`304`	`305`	`if (TraverseGraph(start_nodes, traversed_edge_counts) == INACCURATE) {`
	`306`	`+ inaccurate_results_++;`
`305`	`307`	`return INACCURATE;`
`306`	`308`	`}`
`307`	`309`
`@@ -326,6 +328,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(`
`326`	`328`	`// glyphs that are reachable from any shared_glyphs found above.`
`327`	`329`	`flat_hash_map<Node, unsigned> all_shared_nodes;`
`328`	`330`	`if (TraverseGraph(shared_nodes, all_shared_nodes) == INACCURATE) {`
	`331`	`+ inaccurate_results_++;`
`329`	`332`	`return INACCURATE;`
`330`	`333`	`}`
`331`	`334`
`@@ -339,6 +342,7 @@ StatusOr<DependencyClosure::AnalysisAccuracy> DependencyClosure::AnalyzeSegment(`
`339`	`342`	`}`
`340`	`343`	`exclusive_gids.subtract(or_gids);`
`341`	`344`
	`345`	`+ accurate_results_++;`
`342`	`346`	`return ACCURATE;`
`343`	`347`	`}`
`344`	`348`
Original file line number	Diff line number	Diff line change
`@@ -25,8 +25,6 @@ StatusOr<common::GlyphSet> GlyphClosureCache::GlyphClosure(`
`25`	`25`	`}`
`26`	`26`
`27`	`27`	`glyph_closure_cache_miss_++;`
`28`		`- closure_count_cumulative_++;`
`29`		`- closure_count_delta_++;`
`30`	`28`
`31`	`29`	`hb_subset_input_t* input = hb_subset_input_create_or_fail();`
`32`	`30`	`if (!input) {`
Original file line number	Diff line number	Diff line change
`@@ -130,9 +130,6 @@ Status SegmentationContext::ReassignInitSubset(SubsetDefinition new_def) {`
`130`	`130`
`131`	`131`	`TRYV(GroupGlyphs(changed_gids, changed_segments));`
`132`	`132`
`133`		`- glyph_closure_cache.LogClosureCount(`
`134`		`- "Segmentation reprocess for init def change.");`
`135`		`-`
`136`	`133`	`return absl::OkStatus();`
`137`	`134`	`}`
`138`	`135`
`@@ -186,6 +183,8 @@ Status SegmentationContext::AnalyzeSegment(const SegmentSet& segment_ids,`
`186`	`183`	`PrintDiff("AND", and_gids, dep_and_gids);`
`187`	`184`	`PrintDiff("OR ", or_gids, dep_or_gids);`
`188`	`185`	`PrintDiff("EXC", exclusive_gids, dep_exclusive_gids);`
	`186`	`+ LOG(ERROR) << "init codepoints = " << segmentation_info_->InitFontSegment().codepoints.ToString();`
	`187`	`+ LOG(ERROR) << "init glyphs = " << segmentation_info_->InitFontGlyphs().ToString();`
`189`	`188`	`return absl::InternalError("Depedency graph conditions does not match the closure analysis conditions");`
`190`	`189`	`}`
`191`	`190`	`}`