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

gc-ext: only sweep unmarked objects #45035

Merged
merged 1 commit into from
Apr 20, 2022
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 19, 2022

This prior conditional was a fixed constant branch on !(v | 1), which was always true, since the pointer is never tagged by gc, so this seems more like the intent?

@fingolfin @rbehrends from #28368

This prior conditional was a fixed constant branch, so this seems more like the intent.
@oscardssmith oscardssmith requested a review from chflood April 19, 2022 17:34
@chflood
Copy link
Member

chflood commented Apr 19, 2022

I'm not up on how foreign objects are treated, but in general not sweeping marked objects seems like a good idea.

@fingolfin
Copy link
Member

I don't recall what the rationale for gc_ptr_tag(v, 1) was, and really @rbehrends wrote most of that code; but your analysis sounds right, and I definitely agree that we don't want to sweep marked objects. In fact sweeping marked objects likely would lead to crashes... But right now objects that may need sweeping are rarely if ever used in our code which would explain why we didn't notice such crashes.

@rbehrends
Copy link
Contributor

The change looks correct to me, too, yes. The reason why we probably didn't see crashes in GAP is that the only place where this may actually be used is the HPC-GAP code and some weak pointer code in the baseline GAP GC, neither of which is enabled in the Julia integration.

@fingolfin
Copy link
Member

It is also used by the Semigroups package these days, but again we don't use that by default.

@vtjnash vtjnash added backport 1.6 Change should be backported to release-1.6 backport 1.7 backport 1.8 Change should be backported to release-1.8 labels Apr 20, 2022
@vtjnash vtjnash merged commit ac51add into master Apr 20, 2022
@vtjnash vtjnash deleted the jn/sweep-foreign-unmarked branch April 20, 2022 15:51
KristofferC pushed a commit that referenced this pull request May 16, 2022
This prior conditional was a fixed constant branch, so this seems more like the intent.

(cherry picked from commit ac51add)
@KristofferC KristofferC mentioned this pull request May 16, 2022
45 tasks
KristofferC pushed a commit that referenced this pull request May 16, 2022
This prior conditional was a fixed constant branch, so this seems more like the intent.

(cherry picked from commit ac51add)
@KristofferC KristofferC mentioned this pull request May 16, 2022
67 tasks
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jul 6, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
This prior conditional was a fixed constant branch, so this seems more like the intent.

(cherry picked from commit ac51add)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
This prior conditional was a fixed constant branch, so this seems more like the intent.

(cherry picked from commit ac51add)
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.

5 participants