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

Avoid stack -> Box copy when data can be put in Box directly #101829

Open
jrmuizel opened this issue Sep 14, 2022 · 8 comments
Open

Avoid stack -> Box copy when data can be put in Box directly #101829

jrmuizel opened this issue Sep 14, 2022 · 8 comments
Labels
A-box Area: Our favorite opsem complication A-codegen Area: Code generation C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jrmuizel
Copy link
Contributor

jrmuizel commented Sep 14, 2022

In https://bugzilla.mozilla.org/show_bug.cgi?id=1787715 (currently not public for security reasons) we ran into a stack overflow when flate2 calls Box::<CompressorOxide>>::default() in DeflateBackend::make() here:
https://github.com/rust-lang/flate2-rs/blob/cc5ed7f817cc5e5712b2bb924ed19cab4f389a47/src/ffi/rust.rs#L131

DeflateBackend::make() is getting compiled with a call to chkstk(65664)

A reduced version of this is here:
https://rust.godbolt.org/z/hTE3785xY

pub const LZ_CODE_BUF_SIZE: usize = 64 * 1024;

struct LZOxide {
    pub codes: [u8; LZ_CODE_BUF_SIZE],
}

impl LZOxide {
    #[inline(never)]
    const fn new() -> Self {
        LZOxide {
            codes: [0; LZ_CODE_BUF_SIZE],
        }
    }
}

 pub struct CompressorOxide {
    lz: LZOxide,
    huff: Box<u32>,
    pub codes: [u8; LZ_CODE_BUF_SIZE],
}

impl Default for CompressorOxide {
    #[inline(always)]
    fn default() -> Self {
        CompressorOxide {
            lz: LZOxide::new(),
            huff: Box::default(),
            codes: [0; LZ_CODE_BUF_SIZE],

        }
    }
}

pub fn f() -> Box<CompressorOxide> {
    Box::default()
}

A further reduced version that doesn't require any of the special box syntax in Default() is here:
https://rust.godbolt.org/z/8e5EPafzb

pub const LZ_CODE_BUF_SIZE: usize = 64 * 1024;

struct LZOxide {
    pub codes: [u8; LZ_CODE_BUF_SIZE],
}

impl LZOxide {
    #[inline(never)]
    const fn new() -> Self {
        LZOxide {
            codes: [0; LZ_CODE_BUF_SIZE],
        }
    }
}

 pub struct CompressorOxide {
    lz: LZOxide,
    huff: Box<u32>,
    //pub codes: [u8; LZ_CODE_BUF_SIZE],
}

impl Default for CompressorOxide {
    #[inline(always)]
    fn default() -> Self {
        CompressorOxide {
            lz: LZOxide::new(),
            huff: Box::default(),
            //codes: [0; LZ_CODE_BUF_SIZE],

        }
    }
}

use std::ptr;
use std::alloc::{Layout, alloc};

pub fn g() -> Box<CompressorOxide> {
    
    let layout = Layout::new::<CompressorOxide>();
    let ptr = 
            unsafe { alloc(layout) as *mut CompressorOxide };
            assert!(!ptr.is_null());
            unsafe { ptr::write(ptr, CompressorOxide {
            lz: LZOxide::new(),
            huff: Box::default(),

        });
    Box::from_raw(ptr)}

}

This seems to have to do with LZOxide::new() not being inlined. If it's inlined the problem goes away. If the field initialization order of CompressorOxide is swapped
so that we call Box::<u32>::default() before LZOxide::new() the problem also goes away.

@nikic

@jrmuizel jrmuizel added the C-bug Category: This is a bug. label Sep 14, 2022
@Enselic
Copy link
Member

Enselic commented Jul 11, 2024

Triage: Can you explain a bit more what you want rustc to change here? If you have particular inlining needs, using #[inline(always)] does not seem unreasonable?

@Enselic Enselic added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage-legacy labels Jul 11, 2024
@safinaskar
Copy link
Contributor

@rustbot label A-box

@rustbot rustbot added the A-box Area: Our favorite opsem complication label Feb 6, 2025
@Enselic
Copy link
Member

Enselic commented Feb 6, 2025

Triage: Thanks for adding the label, but at this point I think we can go ahead and close this as obsolete since the reporter did not get back with a clarification.

@Enselic Enselic closed this as not planned Won't fix, can't repro, duplicate, stale Feb 6, 2025
@lqd
Copy link
Member

lqd commented Feb 6, 2025

Why close, is the codegen issue fixed? What the OP wants is to avoid the large copy to the stack to do a heap allocation. That being said, it’s surely far from the only open issue about this.

@Enselic
Copy link
Member

Enselic commented Feb 6, 2025

I think there is an issue about that already which would make this a duplicate. I'll look for it later today. Thanks for clarifying.

@lqd
Copy link
Member

lqd commented Feb 6, 2025

Im sure there are a ton, yes.

@jrmuizel
Copy link
Contributor Author

jrmuizel commented Feb 6, 2025

Why close, is the codegen issue fixed? What the OP wants is to avoid the large copy to the stack to do a heap allocation. That being said, it’s surely far from the only open issue about this.

Yep.

@Enselic Enselic changed the title Box::<CompressorOxide>>::default() creates an LZOxide on the stack and then does a memcpy Do not copy data from the stack to Box when it can be avoided Feb 6, 2025
@Enselic Enselic reopened this Feb 6, 2025
@Enselic Enselic changed the title Do not copy data from the stack to Box when it can be avoided Avoid stack -> Box copy when data can be put in Box directly Feb 6, 2025
@Enselic
Copy link
Member

Enselic commented Feb 6, 2025

I only found #53827 which I don't think is a perfect duplicate. So I

  • reopened this issue
  • retitled it to be more generic (and IMHO descriptive)
  • inlined the code in case godbolt goes down (temporarily or permanently)

Sorry for any inconvenience and thank you for the cooperation. Of course feel free to make whatever further adjustments you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-box Area: Our favorite opsem complication A-codegen Area: Code generation C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants