Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 14, 2023

On 64-bit, we have enough space to encode (1) the tag, (2) the depmods index, and (3) the offset all in a single 64-bit pointer field. This means we don't need the external link_id arrays, which reduces the size of many pkgimages by ~5%.

On 32-bit, we don't have enough bits to implement this strategy. However, most linkages seem to be against the sysimage, and so by giving that a separate tag we can achieve similar compression because the link_id lists will be much shorter.

Fixes: #48218
Closes: #48279

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing backport 1.9 Change should be backported to release-1.9 labels Feb 14, 2023
@vtjnash vtjnash requested a review from timholy February 14, 2023 20:13
@vtjnash vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Feb 15, 2023
@timholy
Copy link
Member

timholy commented Feb 16, 2023

I never did get around to finishing this. Am I understanding you're OK with the uglification compared to #48279? If so I can focus on this branch.

On 64-bit, we have enough space to encode (1) the tag, (2) the
`depmods` index, and (3) the offset all in a single 64-bit pointer
field. This means we don't need the external `link_id` arrays,
which reduces the size of many pkgimages by ~5%.

On 32-bit, we don't have enough bits to implement this strategy.
However, most linkages seem to be against the sysimage, and so
by giving that a separate tag we can achieve similar compression
because the `link_id` lists will be much shorter.
@vtjnash
Copy link
Member Author

vtjnash commented Feb 16, 2023

I didn't realize it wasn't finished before. It is fixed now. I think you will see the new diff is smaller and more complete than the old diff (and I even combined 2 existing tags, so we are net neutral) 🙂

bool external = false;
if (ctx.external_linkage) {
if (jl_object_in_image((jl_value_t*)codeinst)) {
if (0 && jl_object_in_image((jl_value_t*)codeinst)) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to commit this part?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this code is broken, and I accidentally fixed the code that disabled it

Copy link
Member

Choose a reason for hiding this comment

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

Add a comment or something then? Looks very weird to have that standing freely.

Copy link
Member

Choose a reason for hiding this comment

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

Without this code we won't link from package images to the system image. Instead each package image will contain a copy of the native code, so I don't think disabling is an option.

Copy link
Member

Choose a reason for hiding this comment

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

A comment here and in the other places doing this would indeed have been nice...

@vtjnash vtjnash merged commit 8e3e970 into master Feb 17, 2023
@vtjnash vtjnash deleted the teh/compress2 branch February 17, 2023 15:59
@KristofferC
Copy link
Member

A s all size reduction does not seem necessary to backport. And this PR introduces a bunch of dead code so it feels like it needs various follow ups.

@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 17, 2023
@vtjnash
Copy link
Member Author

vtjnash commented Feb 17, 2023

What deadcode is introduced here? I accidentally defined some pointers that used to be null, requiring a more explicit code guard, but that didn't change anything observable (indeed, the point of it was to avoid changing anything notable)

@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Feb 20, 2023
@KristofferC
Copy link
Member

You know better than me but it does look weird to have a bunch of if(0 && ...) spread out around the code base...

@KristofferC KristofferC mentioned this pull request Feb 20, 2023
50 tasks
vtjnash added a commit that referenced this pull request Feb 21, 2023
On 64-bit, we have enough space to encode (1) the tag, (2) the
`depmods` index, and (3) the offset all in a single 64-bit pointer
field. This means we don't need the external `link_id` arrays,
which reduces the size of many pkgimages by ~5%.

On 32-bit, we don't have enough bits to implement this strategy.
However, most linkages seem to be against the sysimage, and so
by giving that a separate tag we can achieve similar compression
because the `link_id` lists will be much shorter.

Co-authored-by: Tim Holy <[email protected]>

(cherry picked from commit 8e3e970)
KristofferC pushed a commit that referenced this pull request Feb 21, 2023
On 64-bit, we have enough space to encode (1) the tag, (2) the
`depmods` index, and (3) the offset all in a single 64-bit pointer
field. This means we don't need the external `link_id` arrays,
which reduces the size of many pkgimages by ~5%.

On 32-bit, we don't have enough bits to implement this strategy.
However, most linkages seem to be against the sysimage, and so
by giving that a separate tag we can achieve similar compression
because the `link_id` lists will be much shorter.

Co-authored-by: Tim Holy <[email protected]>

(cherry picked from commit 8e3e970)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 6, 2023
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.

pkgimages: storage of link_ids should be compressed

5 participants