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

Improve loadtime by removing LazyArray (and be ready for julia 1.9) #189

Merged
merged 9 commits into from
Oct 20, 2022

Conversation

Moelf
Copy link
Member

@Moelf Moelf commented Oct 20, 2022

#1.8.2
time julia --startup-file=no --project=. -e 'using UnROOT; t = LazyTree(UnROOT.samplefile("tree_with_jagged_array.root"), "t1") |> show'
 30  │ [20, 21, 22, 23, 24, 25, 26, 27, 28]
 31  │ []
  
                             69 rows omitted

________________________________________________________
Executed in    8.80 secs    fish           external
   usr time    8.81 secs    0.00 micros    8.81 secs
   sys time    1.53 secs  631.00 micros    1.53 secs

> time ~/Documents/github/julia/julia --startup-file=no --project=. -e 'using UnROOT; t = LazyTree(UnROOT.samplefile("tree_with_jagged_array.root"), "t1") |> show'
...
 29  │ [20, 21, 22, 23, 24, 25, 26, 27]
 30  │ [20, 21, 22, 23, 24, 25, 26, 27, 28]
 31  │ []
  
                             69 rows omitted

________________________________________________________
Executed in    5.75 secs    fish           external
   usr time    5.82 secs  675.00 micros    5.82 secs
   sys time    1.50 secs    0.00 micros    1.50 secs

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 90.27% // Head: 90.38% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (ee1f8fe) compared to base (f6819e6).
Patch coverage: 80.00% of modified lines in pull request are covered.

❗ Current head ee1f8fe differs from pull request most recent head a50dd83. Consider uploading reports for the commit a50dd83 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   90.27%   90.38%   +0.11%     
==========================================
  Files          11       11              
  Lines        1625     1623       -2     
==========================================
  Hits         1467     1467              
+ Misses        158      156       -2     
Impacted Files Coverage Δ
src/UnROOT.jl 100.00% <ø> (ø)
src/iteration.jl 89.65% <80.00%> (+0.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vchuravy
Copy link

What is the timing split between using and doing the task?

@Moelf
Copy link
Member Author

Moelf commented Oct 20, 2022

yeah @vchuravy got it, the problem is LazyArray ('s dependency)

julia> @time_imports using UnROOT
      0.9 ms  Statistics
    156.3 ms  FillArrays
      2.2 ms  StaticArraysCore
    571.9 ms  StaticArrays
   1171.1 ms  ArrayLayouts

and adding usage of LazyArray.Vcat in the precompile block doesn't help with this using time, no surprise I guess. We probably should oust it anyway, since we really just need a lazy Vcat of 1D vector

@vchuravy
Copy link

Yeah we are not expecting using time to be drastically faster.

@Moelf
Copy link
Member Author

Moelf commented Oct 20, 2022

after yeeting the LazyArray:

#1.8.2
  
                             69 rows omitted

________________________________________________________
Executed in    7.19 secs    fish           external
   usr time    7.32 secs    0.00 micros    7.32 secs
   sys time    1.46 secs  515.00 micros    1.46 secs


#1.9 wit the said PR
julia> @time_imports using UnROOT
    543.1 ms  StaticArrays

 29  │ [20, 21, 22, 23, 24, 25, 26, 27]
 30  │ [20, 21, 22, 23, 24, 25, 26, 27, 28]
 31  │ []
  
                             69 rows omitted

________________________________________________________
Executed in    4.05 secs    fish           external
   usr time    4.26 secs   11.66 millis    4.25 secs
   sys time    1.43 secs    3.16 millis    1.42 secs

@Moelf Moelf changed the title Test julia 1.9 ability Improve loadtime by removing LazyArray (and be ready for julia 1.9) Oct 20, 2022
@Moelf Moelf merged commit c025365 into master Oct 20, 2022
@Moelf Moelf deleted the reduce_latency_julia19 branch October 20, 2022 20:46
@@ -44,4 +44,12 @@ include("iteration.jl")
include("custom.jl")
include("displays.jl")

@static if VERSION >= v"1.9"
Copy link
Member

@giordano giordano Oct 25, 2022

Choose a reason for hiding this comment

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

This should help for any version of julia, no? Also, top-level @static does nothing: top-level expressions in a package are evaluated at compile-time anyway.

Copy link
Member Author

@Moelf Moelf Oct 25, 2022

Choose a reason for hiding this comment

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

this doesn't help for <1.9

@Moelf
Copy link
Member Author

Moelf commented Dec 29, 2022

got even faster after JuliaLang/julia#47184

> time ~/Documents/github/julia/julia --startup-file=no --project=. -e 'using UnROOT; t = LazyTree(UnROOT.samplefile("tree_with_jagged_array.root"), "t1") |> show'
 Row │ int32_array
     │ SubArray{Int32,
─────┼──────────────────────────────────────
 1   │ []
 2   │ [0]
                             69 rows omitted

________________________________________________________
Executed in    2.61 secs    fish           external
   usr time    2.52 secs  638.00 micros    2.52 secs
   sys time    0.09 secs    0.00 micros    0.09 secs

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