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

Allow getproperty for PointerWrappers with special structs #74

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

JoshuaLampert
Copy link
Member

This addresses the issue from #72. I'm not 100% sure if this is the proper way to solve the issue since the try ... catch seems a bit unsafe to me. But at least the MWE from the issue (and tests) pass. What do you think?

@sloede
Copy link
Member

sloede commented Mar 26, 2023

IMHO this is not the right way to go about this, since it uses exception handling for control flow.

If this (or something similar) is considered the right way to go about it, we might have to use @generated functions to avoid the runtime overhead of figuring out the type information.

@codecov
Copy link

codecov bot commented Mar 26, 2023

Codecov Report

Merging #74 (9620cec) into main (aa66e86) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   15.42%   15.49%   +0.07%     
==========================================
  Files           3        3              
  Lines        1511     1511              
==========================================
+ Hits          233      234       +1     
+ Misses       1278     1277       -1     
Flag Coverage Δ
unittests 15.49% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pointerwrappers.jl 94.12% <100.00%> (+5.88%) ⬆️

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

@Gnimuc
Copy link

Gnimuc commented Mar 26, 2023

for ty = (:p4est_quadrant, :p4est_etc) # I assume this list is not very large
    @eval Base.getproperty(pw::PointerWrapper{$ty}, name::Symbol) = PointerWrapper(Base.getproperty(pw.pointer, name))
end

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

I agree with @sloede that using exception handling here does not feel good to me. Could you please benchmark it to see whether it introduces an efficiency penalty?
Alternatively, we might investigate falling back to getproperty if findfirst returns nothing - we should of course check whether it's safe and efficient to do so.

@JoshuaLampert
Copy link
Member Author

I'm personally not a great fan of explicitly listing all of these structs manually. When we add more of these structs or some struct, for whatever reason, is renamed we would have to adapt the list. A solution without an explicit list of struct names feels more dynamic to me.

@JoshuaLampert
Copy link
Member Author

JoshuaLampert commented Mar 26, 2023

@ranocha, yes, I can do that. But later because I am at my phone now.

@Gnimuc
Copy link

Gnimuc commented Mar 26, 2023

you could use Clang.jl to emit that list automatically.

if you prefer dynamic over type stability, it's ok.

@ranocha
Copy link
Member

ranocha commented Mar 26, 2023

I guess the idea is to look for a solution that works not only for the types provided by P4est.jl at the moment. If we can find a way to make it work nicely (in an efficient and type-stable way) with general types, we would prefer that. If this turns out to be impossible for some reason, we can look into the option to hard-code everything.

@JoshuaLampert
Copy link
Member Author

JoshuaLampert commented Mar 27, 2023

Could you please benchmark it to see whether it introduces an efficiency penalty?

Yes, this would really have a negative impact on performance.
Current main branch:

julia> @benchmark p4est_pw.global_first_position
BenchmarkTools.Trial: 10000 samples with 585 evaluations.
 Range (min  max):  200.578 ns   2.595 μs  ┊ GC (min  max): 0.00%  91.56%
 Time  (median):     203.909 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   208.215 ns ± 47.972 ns  ┊ GC (mean ± σ):  0.45% ±  1.83%

   ▂▄▄▃▆██▇▅▂               ▁▁ ▁▃▄▃▂ ▂▄▅▄▃▁▁▂▂▃▂               ▂
  ▇██████████▇▄▁▁▃▁▄▃▄▅▅▅▅▇██████████████████████▇▇▇▇▆█▆▇▇▇▆▅▅ █
  201 ns        Histogram: log(frequency) by time       227 ns <

 Memory estimate: 32 bytes, allocs estimate: 2.

(EDIT: previous version of) this PR (before e0286aa):

julia> @benchmark p4est_pw.global_first_position
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  264.867 μs  655.660 μs  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     267.045 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   267.765 μs ±   5.380 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

        ▂▆▇█▆▄▁                                                  
  ▂▂▂▃▄▇███████▇▅▄▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▃▃▂▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂ ▃
  265 μs           Histogram: frequency by time          278 μs <

 Memory estimate: 128 bytes, allocs estimate: 5.

New version of PR (after e0286aa):

julia> @benchmark p4est_pw.global_first_position
BenchmarkTools.Trial: 10000 samples with 560 evaluations.
 Range (min  max):  207.205 ns   2.705 μs  ┊ GC (min  max): 0.00%  91.47%
 Time  (median):     211.016 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   215.619 ns ± 50.082 ns  ┊ GC (mean ± σ):  0.46% ±  1.83%

  ▁▄▂▁ ▄██▅▂▁            ▁  ▃▄▃▂ ▃▄▃▂ ▁▂▁▁                     ▂
  ███████████▇▆▅▄▄▃▃▂▄▅▆███████████████████▇▅▇▇▇▆▆▆▆▆▆▄▅▅▄▃▃▅▃ █
  207 ns        Histogram: log(frequency) by time       238 ns <

 Memory estimate: 32 bytes, allocs estimate: 2.

Also as a comparison, when accessing data from a struct that, where isnothing returns true:
Current main branch:

julia> @benchmark p4est_pw.global_first_position[].level
BenchmarkTools.Trial: 10000 samples with 238 evaluations.
 Range (min  max):  314.462 ns    7.558 μs  ┊ GC (min  max): 0.00%  95.28%
 Time  (median):     319.782 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   329.568 ns ± 143.772 ns  ┊ GC (mean ± σ):  0.86% ±  1.90%

   ▂▃▄▇█▇▆▃▁                   ▁▁▂▂▁▁▁▁▁  ▁▂▃▃▃▂    ▂▁▁         ▂
  ▅██████████▇▇▇▆▅▄▄▂▄▅▇▆▆▇▇▆▇███████████████████████████▆▆▇▅▅▅ █
  314 ns        Histogram: log(frequency) by time        368 ns <

 Memory estimate: 80 bytes, allocs estimate: 4.

This PR (after e0286aa):

julia> @benchmark p4est_pw.global_first_position.level[]
BenchmarkTools.Trial: 10000 samples with 198 evaluations.
 Range (min  max):  440.990 ns    8.127 μs  ┊ GC (min  max): 0.00%  93.99%
 Time  (median):     447.672 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   461.089 ns ± 115.727 ns  ┊ GC (mean ± σ):  0.33% ±  1.33%

  ▄█▆▂    ▅▅▃▃▁                                                 ▁
  ████▆▄▃▇█████████▇▆▆▅▅▅▄▃▅▃▅▄▃▂▃▂▂▂▃▃▂▃▃▃▄▂▄▃▅▄▃▄▃▃▃▃▃▄▂▃▂▄▂▃ █
  441 ns        Histogram: log(frequency) by time        650 ns <

 Memory estimate: 64 bytes, allocs estimate: 4.

@ranocha ranocha requested a review from sloede April 5, 2023 14:20
src/pointerwrappers.jl Show resolved Hide resolved
src/pointerwrappers.jl Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

LGTM!

@sloede sloede enabled auto-merge (squash) April 11, 2023 13:54
@sloede sloede disabled auto-merge April 11, 2023 13:54
@sloede sloede merged commit 177d26e into trixi-framework:main Apr 11, 2023
@JoshuaLampert JoshuaLampert deleted the pw_getproperty branch April 11, 2023 13:55
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.

4 participants