Optimize findall(f, ::AbstractArray{Bool}) #42202
Conversation
|
Out of curiosity, do you know what caused the regression in the first place? |
|
Appears to be #37177, which sped up findall by instantiating a vector of bool (or rather, a bitvector) firsts and working on that. Of course, that is needless when the input is already a vector of bool. |
|
I don't understand the whitespace CI failure. There is no trailing whitespace. Running |
You need to rebase on master to get the fix. |
jakobnissen
force-pushed
the
findallbool
branch
2 times, most recently
from
8340cc9
to
2aa6627
Compare
|
Okay, should be good now. |
Codecov Report
@@ Coverage Diff @@
## master #42202 +/- ##
==========================================
+ Coverage 89.16% 89.28% +0.11%
==========================================
Files 343 343
Lines 78671 79375 +704
==========================================
+ Hits 70146 70867 +721
+ Misses 8525 8508 -17
Continue to review full report at Codecov.
|
|
What this PR needs now is a decision on two details:
If both points are fine, this can be merged. |
jakobnissen
force-pushed
the
findallbool
branch
from
edbfc72
to
9cfa105
Compare
* Take shortcuts if f(::Bool) always returns true or false * Avoid branching in main loop to please branch predictor * Switch to indexing-agnostic code * Fix regression mentioned in JuliaLang#42187
Co-authored-by: Milan Bouchet-Valat <[email protected]>
jakobnissen
force-pushed
the
findallbool
branch
from
6814f8d
to
3664ba3
Compare
oscardssmith
added
the
forget me not
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
@nanosoldier |
|
Any remaining discussion on this? The nanosolidier benchmark/pkgeval look good to me. |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
The failure within PointwiseKDEs seems real. |
|
I can look at it tomorrow if necessary :) |
|
Yes please, I didn't follow the details. |
|
I think the failure of PointwiseKDEs is unrelated:
|
|
We don't usually run all packages' testsuites before merging PRs anyway. I don't see any reason why this one would be breaking. |
DilumAluthge
removed
the
forget me not
This reverts commit 4c4c94f.
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
Fix #42187
The code is slightly more complicated than before, but faster than before the regression.
Also added more tests to cover some cases that was not covered before.
Benchmark (updated 2022-01-08):