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

Create an explicit immutable and cown region. #41

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Conversation

mjp41
Copy link
Collaborator

@mjp41 mjp41 commented Oct 25, 2024

No description provided.

@mjp41 mjp41 requested a review from xFrednet October 25, 2024 14:09
@mjp41 mjp41 removed the request for review from xFrednet October 25, 2024 14:19
@mjp41
Copy link
Collaborator Author

mjp41 commented Oct 25, 2024

I need to fix the cown stuff, before this change can land as I have made it stricter, and properly broken valid_01.vpy, rather than just in Asan

@mjp41 mjp41 changed the title Create an explicit immutable region. Create an explicit immutable and cown region. Oct 25, 2024
@mjp41 mjp41 requested a review from xFrednet October 25, 2024 15:53
@mjp41
Copy link
Collaborator Author

mjp41 commented Oct 25, 2024

I've created a cown region, and it fixes the Asan issue in the cowns/valid_01.vpy test. But I am not sure if it is the right thing from the point of view of understanding.

Pros

  • Naturally captures parenting of the region, into a cown.

Cons

  • Looks messy as all the cowns get grouped together

Alternatives

Could create a new region for each cown. This would look nicer, but requires more complex checking.

Questions

  • Is Opaque the same as Cown? What other flexibility are we expecting here?
  • Should string, true, false, be initially allocated in the immutable region?

@xFrednet
Copy link
Collaborator

Is Opaque the same as Cown? What other flexibility are we expecting here?

In our prototype they are currently the same. I added the is_opaque as a separate method to is_cown to better indicate why an access may be invalid. The methods act the same in this prototype, since we can't acquire a cown. But if we ever add something like acquisition to the prototype, the two functions might return different values.

Another thing I could think of would be closed regions. They would be opaque, until they're opened. (This opening could be implicit though)

@xFrednet
Copy link
Collaborator

xFrednet commented Oct 25, 2024

Looks messy as all the cowns get grouped together

We could try not putting cowns into a sub graph, that would have them floating around, like the global objects initially did. Then we can just say that everything outside a "drawn" region is the cown region. Finding a good visualization will probably also require a bit of try and error :D

We shouldn't block this PR on it, I can try to figure something out next week, if you don't want to :D

Copy link
Collaborator

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice changes! Makes the whole thing a lot cleaner :D

}

void freeze()
{
// TODO SCC algorithm
visit(this, [](Edge e) {
auto obj = e.target;
if (!obj || obj->is_immutable())
if (!obj || obj->is_immutable() || obj->is_cown())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that cowns can't be frozen. It makes sense, but we should check that this is inline with what is written up. If not, we probably want to correct the text on this one, as I like this implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I viewed them as already in the "shared" world, so freeze doesn't do anything. A frozen cown can still be mutated in "my view" of the model.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree. This was just meant as a reminder to check/sync the docs

@mjp41 mjp41 merged commit 899f9a6 into main Oct 25, 2024
5 checks passed
@mjp41 mjp41 deleted the immutable_region branch October 25, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants