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

Implement MallocSizeOf for euclid types #541

Merged
merged 5 commits into from
Feb 19, 2025
Merged

Implement MallocSizeOf for euclid types #541

merged 5 commits into from
Feb 19, 2025

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Feb 14, 2025

Now that malloc_size_of is published to crates.io we are moving the trait implementations of MallocSizeOf and related traits into the crates that define the types. This will avoid the situation where depending on malloc_size_of pulls in a large number of dependencies.

This PR adds the implementation of MallocSizeOf to the euclid crate.

@nical
Copy link
Contributor

nical commented Feb 17, 2025

As usual my main ask is that the dependency does not introduce breaking changes in the future. If changes to MallocSizeOf are planned, then either:

  • Wait for these changes to happen and commit to MallocSizeOf stability in the long run.
  • Don't treat future MallocSizeOf version bumps in euclid as a breaking changes. It would probably make things more difficult for folks who depend on MallocSizeOf + euclid, but it's not insurmountable and will spare everybody else.

I see that CI is having issues probably because the dep keyword is not supported in the old rustc version currently tested. I think that it's fine to update it to a newer compiler if it isn't something too recent. I typically use whatever is in the current rustc package in debian stable (currently 1.63) as a rule of thumb for old enough that an MRSV bump shouldn't be too disruptive.

@nicoburns
Copy link
Contributor Author

If changes to MallocSizeOf are planned, then either wait for these changes to happen and commit to MallocSizeOf stability in the long run.

Changes to MallocSizeOf are not planned (there might be some extra stdlib impls added, but those will be non-breaking). Part of the reasoning around publishing it was that the core of the crate has been stable for several years now, and it was a lot of busywork to continually update this crate just to update impls to new versions of crates. This new version removes all impls for crates other than core/alloc/std and an exception for void because we judge that will almost certainly never release a new version.

Don't treat future MallocSizeOf version bumps in euclid as a breaking changes. It would probably make things more difficult for folks who depend on MallocSizeOf + euclid, but it's not insurmountable and will spare everybody else.

It's mostly Servo and Gecko who will be consuming this. So we could fall back on this if it came to it.

I see that CI is having issues probably because the dep keyword is not supported in the old rustc version currently tested.

Ah, I'll make it use the older form. Although I've noticed a few crates failing CI recently because libm has recently bumped MSRV to 1.63. So that probably is a sensible baseline.

@nical
Copy link
Contributor

nical commented Feb 19, 2025

Alright, thanks for your understanding.

@mrobinson mrobinson added this pull request to the merge queue Feb 19, 2025
Merged via the queue into main with commit 2500741 Feb 19, 2025
20 checks passed
@mrobinson mrobinson deleted the malloc_size_of branch February 19, 2025 11:02
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.

4 participants