Skip to content

Filestore: Use binary search for sequence-from-timestamp lookups#7357

Merged
neilalexander merged 2 commits intomainfrom
neil/getseqfromtime
Sep 26, 2025
Merged

Filestore: Use binary search for sequence-from-timestamp lookups#7357
neilalexander merged 2 commits intomainfrom
neil/getseqfromtime

Conversation

@neilalexander
Copy link
Copy Markdown
Member

@neilalexander neilalexander commented Sep 26, 2025

This updates the filestore GetSeqFromTime function to use a binary search, such that the computational complexity is more predictable. This closes #7352 and fixes #7353 as this is a more idiomatic approach.

Beforehand, the benchmark was very fast for earlier timestamps (as expected with a linear scan) but comparatively glacial for later ones:

go test -v ./server -run=XXX -bench=BenchmarkFileStoreGetSeqFromTime
goos: darwin
goarch: arm64
pkg: github.com/nats-io/nats-server/v2/server
cpu: Apple M2 Ultra
BenchmarkFileStoreGetSeqFromTime
BenchmarkFileStoreGetSeqFromTime/Start
BenchmarkFileStoreGetSeqFromTime/Start-24         	18652426	        61.56 ns/op	       0 B/op	       0 allocs/op
BenchmarkFileStoreGetSeqFromTime/Middle
BenchmarkFileStoreGetSeqFromTime/Middle-24        	  114956	     10275 ns/op	       2 B/op	       0 allocs/op
BenchmarkFileStoreGetSeqFromTime/End
BenchmarkFileStoreGetSeqFromTime/End-24           	   60656	     19740 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/nats-io/nats-server/v2/server	11.673s

After the change, this is now more predictable in the entire range:

go test -v ./server -run=XXX -bench=BenchmarkFileStoreGetSeqFromTime
goos: darwin
goarch: arm64
pkg: github.com/nats-io/nats-server/v2/server
cpu: Apple M2 Ultra
BenchmarkFileStoreGetSeqFromTime
BenchmarkFileStoreGetSeqFromTime/Start
BenchmarkFileStoreGetSeqFromTime/Start-24         	 9555956	       130.8 ns/op	       0 B/op	       0 allocs/op
BenchmarkFileStoreGetSeqFromTime/Middle
BenchmarkFileStoreGetSeqFromTime/Middle-24        	 9335430	       127.4 ns/op	       0 B/op	       0 allocs/op
BenchmarkFileStoreGetSeqFromTime/End
BenchmarkFileStoreGetSeqFromTime/End-24           	10001110	       118.6 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/nats-io/nats-server/v2/server	11.296s

Signed-off-by: Neil Twigg neil@nats.io

@neilalexander neilalexander requested a review from a team as a code owner September 26, 2025 09:30
Copy link
Copy Markdown
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@sciascid sciascid left a comment

Choose a reason for hiding this comment

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

LGTM

defer fs.Stop()

for range 4096 {
_, _, err := fs.StoreMsg("foo.bar", nil, []byte{1, 2, 3, 4, 5}, 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could use StoreMsgRaw() and pass a timestamp, and avoid the time.Sleep() at the end of the loop.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't really need to specify our own timestamps as even the nanosecond precision should be enough (the small block size ensures every message gets its own block), but good spot that I left the sleep in, I will remove it.

Replaces #7352
Fixes #7353

Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
@neilalexander neilalexander requested a review from a team as a code owner September 26, 2025 09:59
Copy link
Copy Markdown
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit 303b1cf into main Sep 26, 2025
90 of 92 checks passed
@neilalexander neilalexander deleted the neil/getseqfromtime branch September 26, 2025 10:22
@darioalessandro
Copy link
Copy Markdown

Cool 😄 does a nats image is generated upon merging this? Where can I get it?

@neilalexander
Copy link
Copy Markdown
Member Author

If it's a Docker image you'd like to test, I can kick off an early rebuild of the nightly image?

@darioalessandro
Copy link
Copy Markdown

Yes, that would be awesome 🙏

@neilalexander
Copy link
Copy Markdown
Member Author

No problem, the updated nightly has been pushed: https://hub.docker.com/r/synadia/nats-server/tags

@darioalessandro
Copy link
Copy Markdown

darioalessandro commented Sep 27, 2025

@neilalexander may I have an arm image? all our nats servers have arm processors and this would help me to test faster.

Also would it be possible to cherrypick this fix into 2.11 and 2.12 please?

neilalexander added a commit that referenced this pull request Sep 29, 2025
Includes the following:

- #7290
- #7295
- #7291
- #7287
- #7299
- #7300
- #7297
- #7303
- #7304
- #7305
- #7309
- #7307
- #7320
- #7337
- #7344
- #7345
- #7348
- #7349
- #7350
- #7357
- #7356
- #7358
- #7367
- #7293

Signed-off-by: Neil Twigg <neil@nats.io>
@darioalessandro
Copy link
Copy Markdown

darioalessandro commented Sep 29, 2025

thanks @neilalexander !!!

neilalexander added a commit that referenced this pull request Sep 29, 2025
Includes the following:

- #7337
- #7342
- #7344
- #7345
- #7347
- #7346
- #7348
- #7349
- #7350
- #7357
- #7356
- #7358
- #7359
- #7366
- #7367
- #7293
- #7368

Signed-off-by: Neil Twigg <neil@nats.io>
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.

JetStream timestamp seek: make GetSeqFromTime O(log n) and fix nil-mid deletion edge case

4 participants