nullable arrays regression regression due to #19760

[StefanKarpinski]

#19760 caused a regression in nullable arrays performance; see this report:

https://github.com/JuliaCI/BaseBenchmarkReports/blob/88d896b0db239199f97014dad2444ed5a1ba6011/6d6ed95_vs_19f81ac/report.md

[Shade5]

Any hint on how to start?

[StefanKarpinski]

This is the benchmark that had a regression:

https://github.com/JuliaCI/BaseBenchmarks.jl/blob/4f6b7d5e66eec616a0cb08dbdee5dc9dbfbd52b4/src/nullable/NullableBenchmarks.jl#L135

The steps are:

0. Reproduce the regression and make sure it's real
1. Figure out how that change affected the performance of that benchmark
2. Figure out how to avoid that performance regression

Feel free to ask questions here while working on it.

[nalimilan]

That's funny, since #19760 only touches BitArray code, but the NullableArray test type uses Array{Bool}...

[Shade5]

These are the commands I executed. I got no error

julia> using BaseBenchmarks
julia> BaseBenchmarks.SUITE
julia> BaseBenchmarks.load!("nullable")
julia> run(BaseBenchmarks.SUITE["nullable"]["nullablearray"]);

[nalimilan]

You're not supposed to get any error, the timings should just be different. See https://github.com/JuliaCI/BaseBenchmarks.jl

[Shade5]

How do I get the jdl for before #19760 ?

[KristofferC]

You don't need it. Please read through https://github.com/JuliaCI/BenchmarkTools.jl to be more familiar with the benchmarking system. In order for more people to learn from the answers I kindly suggest that further usage questions are directed to https://discourse.julialang.org.

[Shade5]

Should I use Nanosoldier to benchmark?

[KristofferC]

Let's continue the discussion at https://discourse.julialang.org/t/basebenchmark-comparing-pr-with-previous-commit/1303

[Shade5]

Sure

[Shade5]

This is the output I'm getting. I've compared master to just before #19760

52-element BenchmarkTools.BenchmarkGroup:
  tags: []
  ("perf_countequals","Array","Int64") => TrialJudgement(+0.08% => invariant)
  ("perf_countnulls","Array","Float32") => TrialJudgement(+0.00% => invariant)
  ("perf_countequals","NullableArray","Float64") => TrialJudgement(+0.00% => invariant)
  ("perf_sum","NullableArray","Float64") => TrialJudgement(-0.46% => invariant)
  ("perf_countnulls","Array","BigFloat") => TrialJudgement(-1.12% => invariant)
  ("perf_countnulls","Array","Complex{Float64}") => TrialJudgement(+0.34% => invariant)
  ("perf_sum","Array","BigFloat") => TrialJudgement(+6.62% => invariant)
  ("perf_countnulls","NullableArray","BigFloat") => TrialJudgement(-0.02% => invariant)
  ("perf_countnulls","NullableArray","Float64") => TrialJudgement(+0.00% => invariant)
  ("perf_countnulls","NullableArray","Int64") => TrialJudgement(+0.00% => invariant)
  ("
perf_countequals","Array","Float32") => TrialJudgement(-0.59% => invariant)

Not getting the difference in performance as you did @StefanKarpinski

[nalimilan]

Thanks for checking. We should probably run Nanosoldier again to see whether the regression is reproducible there.

[Shade5]

@nanosoldier runbenchmarks("nullablearray", vs = "@c38a5a3044ffd62c585225a201422f95fd18f6dc")

[vchuravy]

You need to have commit access to run run nanosoldier, and I don't think you can do it from an issue. I just started a run from 5d90ab6#commitcomment-20387934

[Shade5]

Cool, thanks

[Shade5]

Nanosoldier gave no performance regressions too https://github.com/JuliaCI/BaseBenchmarkReports/blob/22088c22c64940be6ea0e79526456223fbc6a530/5d90ab6_vs_c38a5a3/report.md

[Shade5]

What do we do now?

[nalimilan]

Looks like the regression actually appeared on master before #19760 was merged:
https://github.com/JuliaCI/BaseBenchmarkReports/blob/master/daily_2017_1_2/report.md

The relevant commit range is 1539061...c38a5a3. I can see no obvious culprit the, but then the broadcast changes touch some fundamental code like promote_op. I guess we're going to have to check these commits individually...

[Shade5]

Could it be noise like here: #19928 (comment)

[tkelman]

I'm going to propose closing this in favor of a targeted comparison of all regressions between 0.5 and 0.6 during feature freeze.