Skip to content

fix: support natural ordering for numbers with >12 digits#652

Merged
stevearc merged 3 commits intostevearc:masterfrom
XeroOl:master
Aug 21, 2025
Merged

fix: support natural ordering for numbers with >12 digits#652
stevearc merged 3 commits intostevearc:masterfrom
XeroOl:master

Conversation

@XeroOl
Copy link
Contributor

@XeroOl XeroOl commented Aug 3, 2025

Changes the column ordering code when view_options.natural_order is enabled, so that it can support larger numbers.

I chose to handle numbers of up to 999 digits, because some cursory research seems to suggest to me that most filesystems cap at 255 bytes per segment, and 999 > 255, so it should be sufficient for all actual filenames.

Performance may have changed but I don't know if it will be better or worse.

I've slightly increased the complexity of the string.format format specifier, but the final strings will be shorter on average, and string.format no longer has to implicitly coerce int from a string into a number.

before:
image
after:
image

Changes the column ordering code when `view_options.natural_order`
is enabled, so that it can support larger numbers.

The previous 12-digit padding approach breaks for numbers above 12
digits.

This length-prefixed approach can scale to much higher numbers.
I picked %03 (padding 3 digits) because most filesystems don't allow
more than 255 bytes in a path segment, and "255" is 3 digits long.
@github-actions github-actions bot requested a review from stevearc August 3, 2025 00:16
@XeroOl
Copy link
Contributor Author

XeroOl commented Aug 5, 2025

With the original natural order PR you said you did benchmarking. I'm curious what the methodology was, because I want to know if I've improved performance at all

@XeroOl
Copy link
Contributor Author

XeroOl commented Aug 19, 2025

Okay I've run the benchmark (with make benchmark)

On my computer:
No natural ordering: 686ms
Natural ordering (current): 1.6s
Natural ordering (with my changes): 2.0s

So this PR does slow down the code.

If this is a problem I would be happy to work on performance.

@XeroOl
Copy link
Contributor Author

XeroOl commented Aug 20, 2025

Okay! I've fixed the performance! The main problem is that the sort key callback is being called many times repeatedly for each item. I added memoization so that the key only needs to be computed once per sort.

(Also I swapped an unpack with manual indexing, to avoid a luajit trace stitch)

Before this PR:

command time
make benchmark 660ms +- 45ms
make benchmark (with natural sort) 1.6s +- 45ms

After PR:

command time
make benchmark 607ms +- 8.0ms
make benchmark (with natural sort) 783ms +- 32ms

@stevearc
Copy link
Owner

Nice work! I appreciate the iteration on performance

@stevearc stevearc merged commit 07f80ad into stevearc:master Aug 21, 2025
18 of 19 checks passed
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.

2 participants