go-hep / hep

hep is the mono repository holding all of go-hep.org/x/hep packages and tools
https://go-hep.org
BSD 3-Clause "New" or "Revised" License
230 stars 35 forks source link

xrootd: improve read performances 10-fold #923

Closed sbinet closed 2 years ago

sbinet commented 2 years ago

This CL reduces the lock contention on xrootd.File operations.

  $> benchstat ./ref.txt ./new.txt
  name    old time/op    new time/op    delta
  Read-8     67.2s ± 1%      5.4s ± 7%  -91.93%  (p=0.000 n=9+28)

  name    old alloc/op   new alloc/op   delta
  Read-8     343MB ± 0%     341MB ± 0%   -0.78%  (p=0.000 n=8+30)

  name    old allocs/op  new allocs/op  delta
  Read-8      277k ± 0%      288k ± 0%   +3.84%  (p=0.000 n=7+29)

and now:

  $> time root-dump root://ccxrootdgotest.in2p3.fr:9001/tmp/rootio/testdata/SMHiggsToZZTo4L.root > /dev/null

  real  0m7.279s
  user  0m8.221s
  sys   0m1.256s

compared to:

  $> time root-dump https://cern.ch/binet/big-file.root > /dev/null

  real  0m5.454s
  user  0m6.156s
  sys   0m0.228s

Updates #920.

codecov-commenter commented 2 years ago

Codecov Report

Merging #923 (6f88941) into main (c38575f) will increase coverage by 0.01%. The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   73.75%   73.77%   +0.01%     
==========================================
  Files         404      404              
  Lines       48554    48541      -13     
==========================================
  Hits        35812    35812              
+ Misses      10460    10451       -9     
+ Partials     2282     2278       -4     
Impacted Files Coverage Δ
xrootd/file.go 74.60% <84.21%> (+12.76%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c38575f...6f88941. Read the comment docs.

Moelf commented 2 years ago

I want to understand this mutex lock a bit more. Is it not possible to read from a xrootd file handler in parallel?

The lock can be held by an arbitrary number of readers or a single writer. The zero value for a RWMutex is an unlocked mutex.

I think the answer is yes. But I don't know how it plays with the extern C call.

sbinet commented 2 years ago

it should. (IIRC, there's a test for that, with the -race detector). what the mutex protects is the "session ID" (and a cached value of the remote "file info"). "session ID" is some UUID-like identifier used internally by xrootd (client and server) to multiplex operations.

sbinet commented 2 years ago

I think the answer is yes. But I don't know how it plays with the extern C call.

depending on how you call into go-hep/xrootd (presumably by generating a .so that wraps a Go type and exposes C-funcs), it shouldn't be an issue.

Moelf commented 2 years ago

thanks. I would call this function from different threads. I will check if it works by looking at performance I guess

https://github.com/JuliaPackaging/Yggdrasil/blob/38a8c9dd73fae3b39ec76fee0bb96162a1b6f138/X/xrootdgo/bundled/main.go#L59-L67