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.

Sign up for GitHub

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

Jump to bottom

Pyrona: Create RegionObject and RegionMetadata objects #9

Merged
merged 12 commits into from
Dec 2, 2024
Merged

Pyrona: Create RegionObject and RegionMetadata objects #9

merged 12 commits into from
Dec 2, 2024

Conversation

xFrednet
Copy link
Collaborator

@xFrednet xFrednet commented Nov 25, 2024 •
edited
Loading

TODO list:

  • Memory leak since we don't decref the internal dictionary. Decref causes seg fault
  • Segfault when passing a non string object as the name

cc: @TobiasWrigstad

@xFrednet xFrednet requested a review from mjp41 November 25, 2024 11:04
mjp41
mjp41 approved these changes Nov 25, 2024
View reviewed changes
Copy link
Owner

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM

@TobiasWrigstad
Copy link
Collaborator

I addressed Matt's comments now! Also tests added.

mjp41
mjp41 approved these changes Nov 25, 2024
View reviewed changes
Copy link
Owner

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM. Couple more minor conventions.

@xFrednet xFrednet force-pushed the phase3-region-objects branch 2 times, most recently from 4d3d0db to b95cb10 Compare November 25, 2024 17:06
@xFrednet xFrednet marked this pull request as draft November 25, 2024 17:37
@xFrednet xFrednet changed the title Region: Create RegionObject and RegionMetadata objects Region: Create RegionObject and RegionMetadata objects (WIP) Nov 25, 2024
@xFrednet
Copy link
Collaborator Author

Since the cpython style seems to be inconsistent, I searched for it and found Pep7. Most likely not perfect, but a good reference.

mjp41
mjp41 requested changes Nov 28, 2024
View reviewed changes
Copy link
Owner

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Looks good a few minor things, a couple of possible issues.

Objects/regions.c Outdated
@@ -237,8 +314,9 @@ int _Py_CheckRegionInvariant(PyThreadState *tstate)

// Also need to visit the type of the object
// As this isn't covered by the traverse.
// TODO: this might be covered by tp_traverse?
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't seem to be. I don't understand why though.

@xFrednet xFrednet force-pushed the phase3-region-objects branch from 6842db5 to 4551ac8 Compare November 28, 2024 17:06
xFrednet and others added 8 commits November 28, 2024 17:07
@xFrednet @TobiasWrigstad
Region: Create RegionObject and RegionMetadata objects
3c8ee14 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@xFrednet @TobiasWrigstad
Region: Methods for adding, removing and checking obj membership
b9bca26 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@TobiasWrigstad @xFrednet
Region: Minimal test set for region ownership
230a07e 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@xFrednet @TobiasWrigstad
Region: Correct Invariant and enable on Region creation
7a43d6c 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@xFrednet @TobiasWrigstad
Region: Catch invalid cross region refs
489e569 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@xFrednet @TobiasWrigstad
Region: Allow users to store data in Region fields
e808d1f 
Co-authored-by: Tobias Wrigstad <tobias.wrigstad@it.uu.se>
@xFrednet
Region: Review logic changes
954e2c1
@xFrednet
Region: No more warnings
2ef1ef3
@xFrednet xFrednet force-pushed the phase3-region-objects branch from 4551ac8 to 4324ae4 Compare November 28, 2024 17:08
@xFrednet xFrednet force-pushed the phase3-region-objects branch from 4324ae4 to edebab3 Compare November 28, 2024 17:09
@xFrednet
Copy link
Collaborator Author

xFrednet commented Nov 28, 2024 •
edited
Loading

I believe this should be everything. The last three commits address the review comments. (Thank you for the detailed review <3)

The segmentation fault fixes have become part of e808d1f (My rebase was a bit too aggressive sorry)

// TODO: this might be covered by tp_traverse?

Doesn't seem to be. I don't understand why though.

#9 (comment) should be the last pending comment.

mjp41
mjp41 approved these changes Nov 28, 2024
View reviewed changes
Copy link
Owner

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Couple of minor bits

Objects/regions.c Outdated
@@ -809,15 +815,15 @@ static int Region_init(PyRegionObject *self, PyObject *args, PyObject *kwds) {

// Make the region an owner of the bridge object
self->ob_base.ob_region = (Py_uintptr_t) self->metadata;
_Py_MakeImmutable(Py_TYPE(self));
_Py_MakeImmutable((PyObject*)Py_TYPE(self));
Copy link
Owner

Choose a reason for hiding this comment

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

I think there is something that gives the Type directly at the correct type. PyObject_Type(self) But I think it might gives you a reference count, but the current code doesn't.

I am not sure if _Py_MakeImmutable should receive a borrow or an actual rc. We should discuss in the stand up tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Built-in functions in cpython only borrow arguments, meaning they don't modify the ref count, unless they return an object. The actual decref of the arguments is done by the runtime here:

cpython/Python/bytecodes.c

Lines 2773 to 2775 in ee37c2b

for (int i = 0; i < total_args; i++) {
Py_DECREF(args[i]);
}

I've added a doc comment to _Py_MakeImmutable and changed the default return type to None

@xFrednet xFrednet changed the title Region: Create RegionObject and RegionMetadata objects (WIP) Pyrona: Create RegionObject and RegionMetadata objects Nov 29, 2024
@xFrednet xFrednet marked this pull request as ready for review November 29, 2024 11:27
@xFrednet xFrednet requested a review from matajoh November 29, 2024 11:27
@xFrednet
Copy link
Collaborator Author

@matajoh Since we have Py_SET_REGION should we also add a function+macro to get the region? Something like Py_GET_REGION? It feels weird to use a macro for set but not for get.

@xFrednet
Copy link
Collaborator Author

I've now added a Py_GET_REGION macro. It returns Py_uintptr_t it might be cool to return regionmetadata* instead to avoid casts, but IDK the guidelines and would leave such a refactoring for a followup

@xFrednet xFrednet force-pushed the phase3-region-objects branch from bf291ee to 8924b0b Compare November 29, 2024 12:09
matajoh
matajoh reviewed Nov 29, 2024
View reviewed changes
matajoh
matajoh reviewed Nov 29, 2024
View reviewed changes
matajoh
matajoh reviewed Nov 29, 2024
View reviewed changes
@xFrednet
Copy link
Collaborator Author

Thank you for the detail review. A lot of good comments. The last commit should have addressed everything that has been resolved. I tried to explain our decisions on the others and often added comments in the code to document it better.

@xFrednet xFrednet force-pushed the phase3-region-objects branch from 91c5468 to b322148 Compare November 29, 2024 17:31
@xFrednet xFrednet merged commit 9b2c14d into phase3 Dec 2, 2024
3 checks passed
@xFrednet xFrednet deleted the phase3-region-objects branch December 2, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjp41 mjp41 mjp41 approved these changes

@matajoh matajoh matajoh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xFrednet @TobiasWrigstad @mjp41 @matajoh