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

restore type cache pruning in sysimage saving #37650

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Conversation

JeffBezanson
Copy link
Sponsor Member

The code for this had bitrotted --- dump.c initializes builtin_typenames but doesn't use it, and staticdata.c uses it but doesn't initialize it 😂 This makes it do something again. I noticed this when looking into tree shaking, and in the future it will be very useful for that, to avoid spurious references to things that could otherwise be removed.

@JeffBezanson
Copy link
Sponsor Member Author

Hmm, how do we convince the gc analyzer that the system image shenanigans are ok? The errors don't obviously point to changes in this PR so it makes me wonder how it worked before.

@yuyichao
Copy link
Contributor

Aren't all of these running with GC disabled?

@JeffBezanson
Copy link
Sponsor Member Author

Yes.

@yuyichao
Copy link
Contributor

So it seems that JL_GC_DISABLED should fix it, though I have no idea why this wasn't detected as an error before. The control flow and the arguments leading to the error also seems unchanged...

src/staticdata.c Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Might be simpler to just NULL the references now (in a copy of the svec) then save/restore the ->cache fields, instead of removing them and needing to mess with them on startup too?

@JeffBezanson
Copy link
Sponsor Member Author

How exactly would that work? How do we know which entries to NULL if the ->cache fields are also saved?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 24, 2020

This is a mark-and-sweep moving GC, so you'd handle it the same way as the GC handles WeakRef, in between the record and copy steps.

@JeffBezanson
Copy link
Sponsor Member Author

Oh, I think I tried implementing it that way first --- skipping those fields during the serialize_value step. It seemed ugly to me to add a special case there, but maybe it's no worse than the alternative.

@JeffBezanson
Copy link
Sponsor Member Author

Ok, this version does seem nicer to me.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Better, yes, but it's still corrupting the current runtime by sorting the tn->cache field at the wrong time

src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I don't think these types need to be a special case

src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
src/staticdata.c Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Sponsor Member Author

Assertion failed: (jl_svec_len(newa) == sz), function jl_rehash_type_cache, file /usr/home/julia/buildbot/worker/package_freebsd64/build/src/jltypes.c, line 815.

Well, that's unexpected. Gotta love assertions.

@JeffBezanson
Copy link
Sponsor Member Author

Hmm, I suppose the observed max probe distance is insertion-order-dependent, so rehashing to the same size is not guaranteed to work.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 29, 2020

yeah, we might want to do some sort of "replacements" htable, then we can put the correctly rehashed value in there? or just put the sentinel nothing in the table, instead of attempting to erase them entirely?

@JeffBezanson
Copy link
Sponsor Member Author

I'll try that.

@JeffBezanson
Copy link
Sponsor Member Author

Seems to work. What do you think?

@vtjnash vtjnash merged commit 959aa8f into master Oct 30, 2020
@vtjnash vtjnash deleted the jb/prunetypecache branch October 30, 2020 15:10
@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 30, 2020

Cool, looks like this might save 1-2 MB from the image

vtjnash added a commit that referenced this pull request Jan 14, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650
vtjnash added a commit that referenced this pull request Jan 14, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650
vtjnash added a commit that referenced this pull request Jan 14, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650
vtjnash added a commit that referenced this pull request Jan 14, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650
KristofferC pushed a commit that referenced this pull request Jan 19, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650

(cherry picked from commit f91bb74)
KristofferC pushed a commit that referenced this pull request Feb 1, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650

(cherry picked from commit f91bb74)
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs JuliaLang#37650
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs JuliaLang#37650
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Need to sequence operations correctly here. We need to first finish
marking referenced data before we start deleting unreferenced data. This
otherwise can lead to an assertion failure in some situations.

Refs #37650

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

3 participants