Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staticdata: Some refactors for clarity #52852

Merged
merged 1 commit into from
Jan 12, 2024
Merged

staticdata: Some refactors for clarity #52852

merged 1 commit into from
Jan 12, 2024

Commits on Jan 11, 2024

  1. staticdata: Some refactors for clarity

    I was reading this code as part of looking into #52797. To recap, when we serialize
    objects for package images, we classify each method as either internal or external,
    depending on whether the method extends a function in the ambient set of modules (
    system image, other previously loaded package images, etc) or whether it is private
    to the module we're currently serializing (in which case we call it internal).
    
    One of my primary confusions in reading this code was that the `new_specializations`
    object holds only external code instances in most of the code, but towards the end
    of loading, we used to also add any internal code instances that had dependencies on
    other external method instances in order to share the code instance validation code
    between the two cases.
    
    I don't think this design is that great. Conceptually, internal CodeInstances are
    already in the CI cache at the point that validation happens (their max_world is just
    set to 1, so they're not eligible to run). We do guard the cache insertion by a check
    whether a code instance already exists, which also catches this case, but I think
    it's cleaner to just not add any internal code instances to the list and instead
    simply re-activate the code instance in situ.
    
    Another issue with the old code that I didn't observe, but I think is possible in theory
    is that any CodeInstances that are not part of the cache hierarchy (e.g. because
    they were created by external abstract interpreters) should not accidentally get added
    to the cache hierarchy by this code path. I think this was possible in the old code,
    but I didn't observe it.
    
    With this change, `new_specializations` now always only holds external cis, so rename
    it appropriately. While we're at it, also do some other clarity changes.
    Keno committed Jan 11, 2024
    Configuration menu
    Copy the full SHA
    5239727 View commit details
    Browse the repository at this point in the history