Repository navigation
YoungLogWalk: table_len and logged mean different quantities in closure.dynamic_props' two walks, so young_log_an.py compares unlike numbers #9897
Description
YoungLogWalk's fields are not the same quantity in both modes — for one table
gc/young_log.rs's YoungLogWalk is filled in by each side table's full walk
and its minor-scoped walk, and young_log_an.py compares the two. For five of
the six logged tables the two walks compute table_len from the same
expression. For closure.dynamic_props they do not.
| table | full-walk table_len |
young-walk table_len |
same? |
|---|---|---|---|
gc.layout_tables |
masks.len() + typed.len() |
masks.len() + typed.len() |
yes |
object.descriptors |
attr_keys_by_owner.len() + accessor_keys_by_owner.len() |
identical expression | yes |
shapes.families+indices |
families.len() + indices.len() |
identical expression | yes |
object.transition_cache |
TRANSITION_CACHE_SIZE |
TRANSITION_CACHE_SIZE |
yes |
object.shape_cache |
cache.len() |
cache.len() |
yes |
closure.dynamic_props |
deduped OWNER count | props.len() + prototypes.len() + deleted_keys.len() |
no |
scan_closure_dynamic_props_roots_mut collects owners from all three maps,
sort_unstable() + dedup(), and reports owners.len(). The minor-scoped
walk reported the sum of the three map lengths. An owner present in two of
the three maps counts once in one mode and twice in the other.
logged diverges the same way and more sharply: on a full row it is set to
table_len by construction (logged: table_len, visited: table_len), i.e.
"entries considered", while on a young row it is the number of keys the log
actually named. Reading a full row's logged as "still logged" is wrong.
Why it matters
young_log_an.py derives
skipped = table_len * passes - visited
would_visit = visited + skipped
worse-than-full iff visited > table_len * passes
and prints a per-table verdict comparing the modes. For
closure.dynamic_props those expressions mix the two definitions, so the
young-mode denominator is inflated by exactly the multi-map owners.
Direction of the error, which is the part worth knowing: the inflated
table_len makes the reported skip percentage larger than the truth and
makes worse-than-full harder to trigger. So it cuts the safe way for the
revert in #9895, which takes closure.dynamic_props off its young walk on the
strength of "2.2–2.7 % skipped, worse than full on 223/273 and 217/272 minor
cycles". Both numbers are conservative under this bug; the real case for that
revert is stronger, not weaker. No conclusion drawn from those rows needs
revisiting — but a future comparison that happened to run the other way would
need to know.
How it surfaced
gc::tests::young_log_tests::old_closure_entries_survive_a_minor_full_walk
(#9895). The fixture makes ONE old closure owner carrying a dynamic prop and a
deleted key, so it lands in props and in deleted_keys. Under the young
walk that is table_len == 2; under the full walk it is table_len == 1, and
an assertion moved from one to the other failed with:
YoungLogWalk { partial: false, logged: 1, visited: 1, kept: 0, table_len: 1 }
That is a correct full-walk row. kept == 0 is also correct — an old owner
whose only value is a number has nothing minor-relevant, so it is not
re-logged — but it reads like a stale-log symptom next to logged: 1 until
you know logged means "considered" on a full row.
Suggested resolution
Pick one definition per table and use it in both walks — for
closure.dynamic_props the deduped owner count is the meaningful one, since
both walks iterate owners and the per-entry body is scan_closure_owner. If
the sum-of-maps figure is wanted as well, it belongs in its own field rather
than sharing a name whose meaning then depends on the mode. Documenting on
YoungLogWalk that logged is "entries considered" on a full row and "keys
the log named" on a young row would remove the second half of the confusion at
no cost.
Activity