Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xrootd: improve read performances 10-fold #923

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

sbinet
Copy link
Member

@sbinet sbinet commented Mar 8, 2022

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.

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 go-hep#920.
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

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%) ⬆️

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.

@sbinet sbinet merged commit 6f88941 into go-hep:main Mar 8, 2022
@sbinet sbinet deleted the xrootd-perf branch March 8, 2022 12:39
@Moelf
Copy link

Moelf commented Mar 8, 2022

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
Copy link
Member Author

sbinet commented Mar 8, 2022

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
Copy link
Member Author

sbinet commented Mar 8, 2022

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
Copy link

Moelf commented Mar 8, 2022

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

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.

3 participants