Use NonZero in MixedUnit for C strings #15
Conversation
rust-cloud-vms
bot
force-pushed
the
nonzero-for-cstr
branch
from
5507227 to
c22125e
Compare
|
New bench after rebase, still regressing (on ARM):
|
|
Seems to be a result of inlining decisions by the compiler, as sprinkling |
|
Should we try to add some |
|
Yes, I have something in the works... :D |
rust-cloud-vms
bot
force-pushed
the
nonzero-for-cstr
branch
2 times, most recently
from
849c0aa to
02622e1
Compare
|
Now uses inline, but main branch does as well (aarch64):
|
|
And x86-64:
|
|
Today's nightly (28f1c8079 2025-06-24) bench rerun (aarch64):
Some things to notice:
|
|
Today's nightly (28f1c8079 2025-06-24) bench rerun (amd64):
Some things to notice:
|
|
In conclusion it seems that the perf impact of this change is largely neutral. |
|
Should we close it then? |
|
Well, the main reason to do it is to give more guarantees for C strings, which cannot contain nulls. This change makes this guaranteed by the type system. So as perf looks neutral, I think we should do it. |
|
Oh my bad, thought it was about performance. Then seems all good if perf are not negatively impacted. 👍 Please just fix the CI and then seems all good to me. |
|
|
|
Which use cases require stable support? |
|
None as far as I know but considering it's a public crate and that 1.89 will be released in less than 3 months, I suppose we can just wait? |
|
Sure waiting is one options, and given 1.88 is around the corner, it should only be about 6 weeks. Possible alternatives:
|
|
MSRV is working well nowadays with cargo as it prevents to update to a newer version if MSRV is incompatible. So raising MSRV to 1.89 once it's out seems like the best idea to me. |
rust-cloud-vms
bot
force-pushed
the
nonzero-for-cstr
branch
2 times, most recently
from
c3730e8 to
6bd7251
Compare
|
I added the MSRV, which should cause cargo to emit an error against lower versions. I would expect to see that reflected in the stable-checks CI here, but I don't... because I failed to actually push the right change. Now produces: not sure why three times... |
rust-cloud-vms
bot
force-pushed
the
nonzero-for-cstr
branch
from
6bd7251 to
da1db27
Compare
|
@GuillaumeGomez will anyone be disadvantaged if we were to do a release with this change right now? If MSRV is well supported now, then presumably no-one is disadvantaged? |
|
Well, we still need 1.89 to be released. So just 6 more weeks to wait and then we can do a release. However, we can already release the latest changes you made which improved performance if you want? |
|
Why precisely do we need to wait for 1.89 release? |
|
So it continues to work on stable. 1.89 is released in less than 6 weeks unless I missed something so just a bit of patience and we're all good. :) |
|
But stable can continue to use the current release, right? Nothing changes for stable if we release a newer version that they cannot yet use. What am I missing? |
|
Ah that's what you meant! Yeah I suppose so. It's just annoying for the CI. |
|
Would it help if we change the CI temporarily to 1.89 (current beta)? |
|
Ok let's do it. Please also open an issue to change back the CI to stable once 1.89 is released. |
|
Added commit to change CI to beta and add issue #19 to reset it in 6 weeks. |
|
Thanks! cc @Urgau :) |
|
The description seems outdated. It's no longer a regression right?
rust-analyzer official supports being build with the stable toolchain, so releasing a (for now) beta-only version means that we will end-up with a mismatch between rustc and r-a, which seems unfortunate, keeping them at sync seems important to me. |
|
@Urgau performance seems largely unaffected by this change, after we added Can you explain your concern regarding rust analyzer? They can follow 6 weeks later, but I think they may also not have the same release trains, so could do a release using the new version much sooner than rustc. Is there a benefit to rustc also waiting 6 more weeks? |
Thanks for confirming it, could you update the PR/commit description with the latest perf results.
Well I just don't see the reason to rush releasing a new version that doesn't work on stable; and as said it will create a mismatch between rustc and r-a which isn't great, as we try as much as possible to keep dependencies at sync. |
|
@Urgau The reason I'm exploring this path is that I'd like to integrate this into rustc, which is blocked on a release. There's no rush, but from my myopic perspective the benefits to waiting seem very insignificant to nonexistent, which is why I'm trying to probe them a bit. Ideally we should strive, I think, to not unnecessarily block development, however insignificant. I still don't understand what harm there is in rustc and r-a being a bit out of sync over a crate version. (Given the different release cadences of rusc and r-a, one of either master or a release will always have to be out of sync for a bit, after a bump, I think.) However, given your reluctance, I will not push my point any further. |
|
I'm not opposed to merging the PR (with updated PR/commit description), nor releasing a new version, I'm just not seeing any rush at doing so, but if you tell me it's blocking your progress, that a reason enough to go a bit faster and make a new release. |
|
@Urgau thanks, I've now linked the performance runs from the main description. Let me know if you need anything else. |
Make C strings safer, by encoding the absence of nulls in the type system. This is made possible by rust-lang/rust#141001 which should hit stable in 1.89.
Performance seems largely unaffected. See details for arm64 and for amd64.