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

Faster in-place ntoh #101

Closed
wants to merge 2 commits into from
Closed

Faster in-place ntoh #101

wants to merge 2 commits into from

Conversation

Moelf
Copy link
Member

@Moelf Moelf commented Sep 13, 2021

You guys will have to tell me how horrible this PR is.

Before:

julia> using Revise, UnROOT
[ Info: Precompiling UnROOT [3cd96dde-e98d-4713-81e9-a4a1b0235ce9]
cons
julia> const t = LazyTree(ROOTFile("Run2012BC_DoubleMuParked_Muons.root"), "Events", ["Muon_pt"]);

julia> @time  for evt in t
                evt.Muon_pt
              end
  5.021026 seconds (1.93 M allocations: 4.254 GiB, 4.75% gc time, 4.34% compilation time)

julia> @time  for evt in t
                evt.Muon_pt
              end
  4.285786 seconds (58.16 k allocations: 4.159 GiB, 1.68% gc time)

julia> @time  for evt in t
                evt.Muon_pt
              end
  4.255702 seconds (58.16 k allocations: 4.159 GiB, 1.48% gc time)

After:

julia> const t = LazyTree(ROOTFile("Run2012BC_DoubleMuParked_Muons.root"), "Events", ["Muon_pt"]);

julia> @time let x = 0
           for evt in t
               evt.Muon_pt
           end
           x
       end
  4.731603 seconds (1.46 M allocations: 3.674 GiB, 4.55% gc time, 4.94% compilation time)
0

julia> @time  for evt in t
                evt.Muon_pt
              end
  4.096869 seconds (59.20 k allocations: 3.603 GiB, 1.22% gc time)

@Moelf Moelf requested a review from aminnj September 13, 2021 23:57
@Moelf Moelf closed this Sep 14, 2021
@aminnj
Copy link
Member

aminnj commented Sep 14, 2021

Not sure why you closed? Some questions

  1. Does the speedup hold with @btime? Probably easier to see it on the uncompressed file when it's not diluted by 70% zlib.
  2. Won't this mess up the basketarray API? Users will have to keep that reference themselves when it's not used internally via LazyBranch. Or maybe we make that an internal api?

@Moelf
Copy link
Member Author

Moelf commented Sep 14, 2021

Won't this mess up the basketarray API?
yep... which is why I closed it.

I think we should disable array() API. Maybe only allow if for raw = true

@aminnj
Copy link
Member

aminnj commented Sep 14, 2021

Just tested this PR vs master on another machine. I get @btimes ranging from 1.698 to 1.652 when alternately switching between the two. It seems like there's no time difference. My benchmark is

julia> using UnROOT

julia> const t = LazyTree(ROOTFile("uncompressed_Run2012BC_DoubleMuParked_Muons.root"), "Events");

julia> function foo(t)
           s = 0.0f0
           for evt in t
               s += sum(evt.Muon_pt)
           end
           s
       end
foo (generic function with 1 method)

julia> @benchmark foo(t)

@aminnj
Copy link
Member

aminnj commented Sep 14, 2021

With a benchmark where I don't loop over the elements I get

julia> function foo2(t)
           for evt in t
               evt.Muon_pt
           end
       end
foo2 (generic function with 1 method)

julia> @benchmark foo2(t) seconds=15 # master
BenchmarkTools.Trial: 17 samples with 1 evaluation.
 Range (min  max):  851.025 ms  991.843 ms  ┊ GC (min  max): 6.61%  6.04%
 Time  (median):     876.236 ms               ┊ GC (median):    6.92%
 Time  (mean ± σ):   889.033 ms ±  40.918 ms  ┊ GC (mean ± σ):  6.75% ± 0.29%
 Memory estimate: 1.82 GiB, allocs estimate: 14304.

julia> @benchmark foo2(t) seconds=15 # PR
BenchmarkTools.Trial: 17 samples with 1 evaluation.
 Range (min  max):  769.904 ms  873.306 ms  ┊ GC (min  max): 5.60%  5.41%
 Time  (median):     795.748 ms               ┊ GC (median):    5.84%
 Time  (mean ± σ):   800.257 ms ±  23.881 ms  ┊ GC (mean ± σ):  5.78% ± 0.20%
 Memory estimate: 1.26 GiB, allocs estimate: 14602.

but it's kind of a dumb benchmark?

@Moelf
Copy link
Member Author

Moelf commented Sep 14, 2021

at least

Memory estimate: 1.26 GiB

so I think in some cases this would be faster

if J == Nojagg
return ntoh.(reinterpret(T, rawdata))
ϖ = convert(Ptr{eltype(T)}, pointer(rawdata))
Copy link
Member

Choose a reason for hiding this comment

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

ϖ :(

offset[i+1] = offset[i] + l
else
# when we have an empty [] in jagged basket
offset[i+1] = offset[i]
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this else?

@Moelf Moelf reopened this Sep 14, 2021
@Moelf Moelf closed this Sep 14, 2021
@Moelf Moelf deleted the fasterrrr branch July 1, 2022 21:39
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