Skip to content

Replace readwrite protection with read protection #1631

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

Closed
wants to merge 9 commits into from

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Nov 25, 2024

The mutex analysis collects two kinds of protection information: readwrite and write. The former is a subset of the latter.

This PR changes it to collect just read protection instead of readwrite. When asked for readwrite protection, the intersection is still taken to find mutexes that protect both all reads and all writes.

I hoped the protection-read privatization could slightly be improved by using read-protection instead of readwrite-protection for its extra check, but that didn't work out. As-is this is more of a refactoring/cleanup: analysis results aren't really affected, although the read-protection sets can be seen in g2html.

While changing that I realized we currently use write:bool in two different ways:

  1. In queries about protection, write=true means write-protection and write=false means readwrite-protection.
  2. In mutex analysis itself, write=true means a global write and write=false means a global read (only!).

I don't think this causes any bugs but it's still rather confusing because there are no readwrite accesses to globals (separate read and write access events are emitted).
As-is this PR makes things more consistent and correct, although readwrite accesses are never emitted, so this could be encoded more precisely with polymorphic variants.

@sim642 sim642 requested a review from michael-schwarz July 10, 2025 14:38
@sim642
Copy link
Member Author

sim642 commented Jul 11, 2025

Oh no, after fixing conflicts (and adding a cram test for #1712), the evals counts in weak dependencies test have changed.
I should probably look into that before we merge this because I don't think it should happen: the unknowns are all the same.

EDIT: I now see why that's happening and it might not be a good idea to do this, especially since this isn't supposed to improve any precision either. I think I can extract the cleanup part and the fix for #1712 without doing the switch from readwrite to read.

Basically, having more precise data internally means unknowns can change more without any visible effect:

  • Previously:
    1. g starts out with (All mutexes, All mutexes) as readwrite and write protecting.
    2. After an unprotected write, we side-effect ({}, {}), which is a change that destabilizes.
    3. After an unprotected read, we side-effect ({}, All mutexes), which joined with the previous isn't actually a change, so no destabilization.
  • Now:
    1. g starts out with (All mutexes, All mutexes) as read and write protecting.
    2. After an unprotected write, we side-effect (All mutexes, {}), which is a change that destabilizes.
    3. After an unprotected read, we side-effect ({}, All mutexes), which joined with the previous goes to ({}, {}), which is a change that also destabilizes.
    4. Thus, more evals happen.

This is exactly the kind of silly useless re-evaluation that I suspect we might be accidentally doing. We definitely shouldn't add more of those.

@sim642 sim642 marked this pull request as draft July 11, 2025 18:19
@sim642 sim642 self-assigned this Jul 21, 2025
@sim642
Copy link
Member Author

sim642 commented Jul 23, 2025

Closing in favor of #1788.

@sim642 sim642 closed this Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup Refactoring, clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variables protected by mutex are incorrectly calculated
2 participants