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

make branch basket buffer threadlocal #76

Merged
merged 8 commits into from
Sep 4, 2021

Conversation

aminnj
Copy link
Member

@aminnj aminnj commented Sep 3, 2021

In each of the branch objects, the last-read basket data is stored. If one tried to naively @threads an event loop, there would be huge contention in this buffer cache. This PR makes nthreads() such buffer caches.

using UnROOT
using LVCyl
using FHist
using Base.Threads

const f = ROOTFile("Run2012BC_DoubleMuParked_Muons.root")
const t = LazyTree(f, "Events", ["nMuon", r"^Muon_(pt|eta|phi|mass|charge)"])

const hmass = Hist1D(Int64; bins=0:0.001:150)
@time @threads for (i,evt) in enumerate(t)
    evt.nMuon != 2 && continue
    sum(evt.Muon_charge) != 0 && continue
    lvs = LorentzVectorCyl.(evt.Muon_pt, evt.Muon_eta, evt.Muon_phi, evt.Muon_mass)
    mass = sum(lvs).mass
    push!(hmass, mass) # rename to atomic_push eventually
end

gives

27.897073 seconds (49.06 M allocations: 39.579 GiB, 23.23% gc time) # with @threads
68.897655 seconds (48.65 M allocations: 38.559 GiB, 7.48% gc time) # without @threads

for julia with 4 threads on a mac laptop. That's about a 2.5x speedup (consistent with me seeing 300% cpu usage instead of 400%).

Note that I commented out the LRU cache around readbasketseek() in both cases to keep RAM usage low (seems to go beyond the advertised maxsize=1024^3 🤷)

The example comes from #73 .

@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #76 (08380f8) into master (ac38c75) will increase coverage by 0.54%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   82.55%   83.10%   +0.54%     
==========================================
  Files          10       10              
  Lines        1244     1302      +58     
==========================================
+ Hits         1027     1082      +55     
- Misses        217      220       +3     
Impacted Files Coverage Δ
src/iteration.jl 68.46% <75.00%> (+2.35%) ⬆️
src/streamers.jl 90.31% <0.00%> (-0.05%) ⬇️
src/utils.jl 100.00% <0.00%> (ø)
src/custom.jl 48.86% <0.00%> (+0.02%) ⬆️
src/types.jl 92.50% <0.00%> (+0.09%) ⬆️
src/io.jl 93.93% <0.00%> (+0.18%) ⬆️
src/root.jl 90.74% <0.00%> (+0.21%) ⬆️
src/bootstrap.jl 83.56% <0.00%> (+0.23%) ⬆️
src/displays.jl 50.00% <0.00%> (+1.42%) ⬆️

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 ac38c75...08380f8. Read the comment docs.

@tamasgal
Copy link
Member

tamasgal commented Sep 3, 2021

Awesome! Btw. the memory estimation is very basic and does not catch nested data well, that's why it can grow larger than the set 1GB.

@aminnj
Copy link
Member Author

aminnj commented Sep 3, 2021

Since @threads relies on getindex() to do the chunking, this previously wouldn't work:

@threads for (i,evt) in enumerate(t)
    i>5 && break
    evt.nMuon != 2 && continue
    sum(evt.Muon_charge) != 0 && continue
    # ...
end

because enumerate(t) doesn't assume a getindex() interface by default (see here), which is pretty inconvenient/unintuitive if you want to take some test code and slap on @threads. One would have to write

@threads for i in 1:length(t)
    i>5 && break
    t.nMuon[i] != 2 && continue
    sum(t.Muon_charge[i]) != 0 && continue
    # ...
end

However, our LazyTable in indexable, so based on the code here, I added some extra definitions to allow us to @threads ... enumerate(t).

@aminnj
Copy link
Member Author

aminnj commented Sep 4, 2021

As far as features, I think I'm done in this PR. A review is welcome :)

@aminnj aminnj mentioned this pull request Sep 4, 2021
@tamasgal
Copy link
Member

tamasgal commented Sep 4, 2021

Weeeha! 😃

@tamasgal tamasgal merged commit 45607b2 into JuliaHEP:master Sep 4, 2021
peremato pushed a commit to peremato/UnROOT.jl that referenced this pull request Sep 7, 2021
* make branch basket buffer threadlocal

* clean up

* allow getindex to work with enumerate

* avoid reading the whole event

* don't hijack all enumerations

* test looping interfaces

* cover extra methods

* 1.3 doesn't even like the regular loop interface
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