Skip to content

Reduce size of Image and make it consistent no matter the featureset#11655

Open
eira-fransham wants to merge 3 commits into
slint-ui:masterfrom
eira-fransham:reduce-size-of-image
Open

Reduce size of Image and make it consistent no matter the featureset#11655
eira-fransham wants to merge 3 commits into
slint-ui:masterfrom
eira-fransham:reduce-size-of-image

Conversation

@eira-fransham
Copy link
Copy Markdown
Contributor

@eira-fransham eira-fransham commented May 7, 2026

Image being a different size depending on featureset has apparently caused miscompilation before, and it's somewhat of a footgun when including Image in other types which are used in ffi. Another issue is that it bloats the size of Image whether or not the variants are being used. Image is stored unboxed all over the codebase, and particularly in generated code.

Size comparison

  • Before (wgpu enabled): 88 bytes
  • Before (wgpu disabled): 56 bytes
  • After (wgpu enabled or disabled): 24 bytes

A struct or component that has many image fields is wasting up to ~70% of its space storing what is essentially just padding. Consider that Slint does not have a variant/union type, so even in cases where fields are mutually exclusive they cannot reuse space. This means that it's not uncommon in larger applications for structs/components to have many fields sequentially. A downside is that embedded images now have an extra pointer of indirection. From looking at the data actually stored by EmbeddedImage it seems like this is possible to solve, but doing so would require significantly complicating the type and so it's beyond the scope of this PR.

In the future, we may be able to take advantage of alignment niches, which, depending on the alignment of the types in ImageInner, may bring the size all the way down to 16 bytes. Without those "niche" optimisations it's not possible to reduce the size of the type any further without changing how VRc works.

Size on different platforms is not unified, so for example Wasm and native compilation may still result in different sizes of Image (I haven't checked). A static assertion is added, to ensure that ImageInner is the same size as the version of the enum with all features disabled.

My approach involved slightly changing some constructors in the C++ Image bindings, so I'd appreciate if someone could take a look at that side of things

@eira-fransham eira-fransham requested a review from tronical May 7, 2026 15:27
Size on different platforms is not unified, so for example Wasm and native
compilation may still result in different sizes of `Image`
@eira-fransham eira-fransham force-pushed the reduce-size-of-image branch from d00c721 to 55fea54 Compare May 7, 2026 15:41
Copy link
Copy Markdown
Member

@tronical tronical left a comment

Choose a reason for hiding this comment

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

I think it's a good idea to put the wgpu textures behind a VRc - generally speaking.

Comment thread Cargo.toml
license = "GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0"
repository = "https://github.com/slint-ui/slint"
rust-version = "1.92"
rust-version = "1.95"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest to make this change without bumping the MSRV. If we decide to bump it, it's an effort that also requires changing documentation and it's something we do usually very consciously.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I only changed it because I wrote the PR to use if let guards and I thought this would be a quick way to test this on CI since I was mostly busy on my drag-and-drop PR. It ended up breaking CI even more though


/// An embedded image, storing a cache key and the inline image data.
#[derive(Clone, Debug, PartialEq)]
#[repr(C)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this struct need to be repr(C) given that it's now behind the VRc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ooh good point, yeah I don't think it does since we don't generate bindings to it for C++ at all

#[expect(unused)]
const _: () = {
#[repr(u8)]
enum ImageInnerAllFeaturesDisabled {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm unsure if this is going to buy us much, TBH. We had issues with cfgs before here, but they were also related to propagation into cbindgen. So suppose the assertions end up failing here, we'll end up making a change here and that's it - could still introduce a missing #define on the C++ side...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could change it to cfg(all(test, not(target_family = "wasm"))), that means it'll be compiled on CI as a regression test. Also, I could make it explicitly check that the size is the same as:

struct Foo {
  a: u8,
  b: usize,
  c: usize,
}

instead of checking against a facsimile of the enum without features enabled. That should work as a cross-platform regression test at least.

#[cfg_attr(not(feature = "ffi"), i_slint_core_macros::remove_extern)]
#[vtable::vtable]
#[repr(C)]
pub(super) struct DropShimVTable {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not reuse OpaqueImageVTable ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe! I'll have a look on Monday.

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