Skip to content

eliminate internal tracking of EOF#6015

Merged
rnewson merged 1 commit into
mainfrom
no-eof
May 29, 2026
Merged

eliminate internal tracking of EOF#6015
rnewson merged 1 commit into
mainfrom
no-eof

Conversation

@rnewson
Copy link
Copy Markdown
Member

@rnewson rnewson commented May 26, 2026

Overview

Manually tracking is error-prone. calling file:position/2 is very cheap, so just do that when we're writing.

Testing recommendations

Covered by eunit tests.

Related Issues or Pull Requests

N/A

Checklist

  • This is my own work, I did not use AI, LLM's or similar technology
  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • Documentation changes were made in the src/docs folder
  • Documentation changes were backported (separated PR) to affected branches

@rnewson
Copy link
Copy Markdown
Member Author

rnewson commented May 26, 2026

noting this might make the read_beyond_eof handling code unreachable (iirc we added that when we speculated sources of eof tracking being wrong in ways we couldn't reproduce).

@rnewson rnewson requested a review from nickva May 26, 2026 13:00
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

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

I like this change, it simplifies things quite a bit. We'll have to check a bit more thoroughly but a preliminary quick bench shows the extra syscall costs about 3-5 microseconds

macos/intel

>element(1,timer:tc(fun() -> [file:position(Fd, eof) || I <- lists:seq(1,1000000)], ok end))/1000000.
3.739081

debian trixie/intel

f(Fd), {ok, Fd} = file:open("/var/tmp/f.txt", [binary, append, create,raw]).
(dbcore@db2.relengtest001.cloudant.net)5> element(1,timer:tc(fun() -> [file:position(Fd, eof) || I <- lists:seq(1,1000000)], ok end))/1000000.
4.243807

Another thing to keep an eye on is if this could acquire any extra locks or affect concurrency. Say would 10k couch_file doing an extra lseek call hit any possible bottleneck in the os layer (I would wager, not but worth thinking about)

@rnewson rnewson force-pushed the no-eof branch 4 times, most recently from e2300c5 to 4c3fa2e Compare May 28, 2026 10:15
@rnewson rnewson marked this pull request as ready for review May 28, 2026 10:15
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

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

+1 with one minor optimization to reduce syscall count for pread batches

Comment thread src/couch/src/couch_file.erl Outdated
@rnewson rnewson force-pushed the no-eof branch 2 times, most recently from 39b1223 to bc656f7 Compare May 28, 2026 18:59
Copy link
Copy Markdown
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

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

+1

@nickva
Copy link
Copy Markdown
Contributor

nickva commented May 28, 2026

I ran a few k6-couch benchmark runs against main

BENCH_DOCS=100000 BENCH_DURATION=3m BENCH_SCENARIOS=doc_get,doc_insert  k6 run k6_couchdb.js
...
running (3m46.0s), 00000/01000 VUs, 198002 complete and 0 interrupted iterations
doc_get    ✓ [======================================] 0000/0500 VUs  3m0s  1000.00 iters/s
doc_insert ✓ [======================================] 0000/0500 VUs  3m0s  100.00 iters/s

(100k docs, doc get and doc insert for 3 minute)

Main:

       { name:doc_get }.............: avg=1.68ms min=1.06ms med=1.48ms max=116.65ms p(90)=1.8ms  p(95)=1.93ms
      { name:doc_insert }..........: avg=3.95ms min=2.62ms med=3.5ms  max=151.07ms p(90)=4.24ms p(95)=4.57ms

      { name:doc_get }.............: avg=2.03ms min=1.06ms med=1.48ms max=149.24ms p(90)=1.82ms p(95)=1.98ms
      { name:doc_insert }..........: avg=4.6ms  min=2.6ms  med=3.51ms max=170.73ms p(90)=4.29ms p(95)=4.74ms

      { name:doc_get }.............: avg=2.12ms min=1.06ms med=1.49ms max=157.79ms p(90)=1.81ms p(95)=1.99ms
      { name:doc_insert }..........: avg=4.58ms min=2.59ms med=3.51ms max=164.38ms p(90)=4.28ms p(95)=4.68ms

No Eof (PR)

      { name:doc_get }.............: avg=2.25ms min=1.04ms med=1.56ms max=162.39ms p(90)=1.98ms p(95)=2.23ms
      { name:doc_insert }..........: avg=5.15ms min=2.67ms med=3.97ms max=193.4ms  p(90)=5.11ms p(95)=5.93ms

      { name:doc_get }.............: avg=1.85ms min=1.07ms med=1.52ms max=176.15ms p(90)=1.89ms p(95)=2.05ms
      { name:doc_insert }..........: avg=4.49ms min=2.73ms med=3.85ms max=210.74ms p(90)=4.8ms  p(95)=5.29ms

      { name:doc_get }.............: avg=1.77ms min=1.07ms med=1.53ms max=148.22ms p(90)=1.9ms  p(95)=2.07ms
      { name:doc_insert }..........: avg=4.4ms  min=2.76ms med=3.88ms max=123.9ms  p(90)=4.79ms p(95)=5.31ms

The difference is there but is tiny:

Medians for doc_get
main: 1.48, 1.48,1.49
pr: 1.56,1.52,1.53

Medians for doc_insert:
main: 3.5, 3.51, 3.51
pr: 3.97, 3.85, 3.88

So maybe 0.1-0.5 msec difference

P90 for doc_get:
main: 1.8, 1.82, 1.81
pr: 1.98, 1.89, 1.9

P90 for doc_insert:
main: 4.24, 4.29, 4.28
pr: 5.11, 4.8, 4.79

About the same for reads and <1msec difference for writes

@rnewson rnewson force-pushed the no-eof branch 3 times, most recently from bd3afbf to 08405d1 Compare May 29, 2026 12:31
@rnewson rnewson merged commit 03d6065 into main May 29, 2026
63 checks passed
@rnewson rnewson deleted the no-eof branch May 29, 2026 20:10
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