Skip to content

YoungLogWalk: table_len and logged mean different quantities in closure.dynamic_props' two walks, so young_log_an.py compares unlike numbers #9897

Description

@proggeramlug

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions