Skip to content

feat: remove "alloc" feature requirement from base_convert/fmt#488

Merged
prestwich merged 2 commits intorecmo:mainfrom
DaniPopes:no-alloc-fmt-2
Jul 17, 2025
Merged

feat: remove "alloc" feature requirement from base_convert/fmt#488
prestwich merged 2 commits intorecmo:mainfrom
DaniPopes:no-alloc-fmt-2

Conversation

@DaniPopes
Copy link
Contributor

@DaniPopes DaniPopes commented Jul 5, 2025

Motivation

Alternative to #478 by making to_base_be not allocate directly.
Closes #478.

Needs #487 to gauge perf impact.

Closes #294.

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 5, 2025

CodSpeed Performance Report

Merging #488 will not alter performance

Comparing DaniPopes:no-alloc-fmt-2 (d349967) with main (fb6460a)

Summary

✅ 295 untouched benchmarks

@DaniPopes DaniPopes force-pushed the no-alloc-fmt-2 branch 2 times, most recently from f21de73 to 12eeb0d Compare July 6, 2025 03:06
@DaniPopes DaniPopes force-pushed the no-alloc-fmt-2 branch 5 times, most recently from 7c97013 to 6d32165 Compare July 7, 2025 17:26
@DaniPopes
Copy link
Contributor Author

As mentioned in #490 this a lot slower because of precomputing the highest power, then dividing that power and (bigint) division is slow; it looks like just computing le and reversing iteration is almost always faster. Do we want to feature gate the slow implementation to not(feature = "alloc") and keep the collect one otherwise?

@DaniPopes DaniPopes marked this pull request as ready for review July 7, 2025 18:03
@DaniPopes DaniPopes requested a review from prestwich as a code owner July 7, 2025 18:03
@DaniPopes DaniPopes force-pushed the no-alloc-fmt-2 branch 4 times, most recently from 546965f to 2865594 Compare July 14, 2025 19:53
@prestwich
Copy link
Collaborator

As mentioned in #490 this a lot slower because of precomputing the highest power, then dividing that power and (bigint) division is slow; it looks like just computing le and reversing iteration is almost always faster. Do we want to feature gate the slow implementation to not(feature = "alloc") and keep the collect one otherwise?

Yes, i would appreciate this

@DaniPopes
Copy link
Contributor Author

Done

@DaniPopes DaniPopes changed the title feat: remove heap-allocation from base_convert/fmt feat: remove "alloc" feature requirement from base_convert/fmt Jul 17, 2025
@DaniPopes
Copy link
Contributor Author

DaniPopes commented Jul 17, 2025

the performance improvement in fmt/4096 is due to hoisting the self.base load to outside of the loop which has drastic consequences to the generated code: https://godbolt.org/z/Wzbsbf4Pb

and the ones for 64 and 128 are due to the added fast paths

i'll move these changes to a separate PR

@prestwich prestwich merged commit 7bf292e into recmo:main Jul 17, 2025
19 checks passed
@prestwich prestwich mentioned this pull request Jul 17, 2025
3 tasks
@DaniPopes DaniPopes deleted the no-alloc-fmt-2 branch July 19, 2025 12:04
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.

base_convert: Find an allocation free method. Maybe extract from the top?

2 participants