fix for loop for array indexing #15497

meggart · 2016-03-14T13:53:58Z

This is the iteration fixes for the files abstractarray.jl ... arraymath.jl, see issue #15434

There were quite a few cases where the fix was not obviuos because of one of these reasons:

there was an offset in the iteration e.g. (for i=2:length(A))
iteration was done over certain dimensions of a matrix
changes were too non-trivial for this PR, I did not want to touch (and possibly mess up) map_n! and map_to_n!

However, #Fixme iter and #Fixme comp comments were added in these cases.

nalimilan · 2016-03-14T14:26:27Z

base/array.jl

@@ -833,7 +833,7 @@ end
 function indcopy(sz::Dims, I::Vector)
    n = length(I)
    s = sz[n]
-    for i = n+1:length(sz)
+    for i = n+1:length(sz) #Fixme iter, might be unnecessary, because it is not exported


This is OK here since I is restricted to Vector (as opposed to AbstractVector).

nalimilan · 2016-03-14T14:49:22Z

Thanks! I think most of the FIXMEs can be fixed easily, though some of them require more though/work/features.

meggart · 2016-03-14T16:10:23Z

Thanks for the review. I have removed some FIXME comments and updated some type signatures according to your remarks.

IainNZ · 2016-03-14T19:17:08Z

Does all this ziping slow down the code at all for plain-old-arrrays?

nalimilan · 2016-03-14T22:26:51Z

Does all this ziping slow down the code at all for plain-old-arrrays?

It doesn't (well, currently almost not, and with a special iterator type not at all): #15356 (comment)

tkelman · 2016-03-15T11:16:51Z

base/abstractarraymath.jl

@@ -179,8 +180,8 @@ end

 function ipermutedims(A::AbstractArray,perm)
    iperm = Array(Int,length(perm))
-    for i = 1:length(perm)
-        iperm[perm[i]] = i
+    for (i,j) = zip(1:length(perm),eachindex(perm))


is this the same as enumerate(eachindex(perm)) ?

Yes, would the enumerate version be preferred?

meggart · 2016-03-17T09:17:03Z

PR is updated according to the comments.

meggart · 2016-03-17T09:31:11Z

Sorry for the confusion, changes were still local...

musm · 2016-03-17T13:30:18Z

base/array.jl

-    for i = 1:length(A)
-        if testf(A[i])
+    for (i,j) = enumerate(eachindex(A))
+        if testf(A[j])


why is the enumerate(eachindex(A)) preferable over the straightforward i=1:length(A)?

Because some package might implement a non-standard Fortran-style indexing, where indices have an offset and don't start at 1.
Also Linear indexing might be slow if A is a SubArray.

Makes sense, although makes the code kinda ugly.

You should be able to use enumerate(A) directly.

In fact the code seems to have a bug as written. It should be something like

for (i, a) in enumerate(A) if testf(a)

(In other words, a = A[i].)

I see, one doesn't need the index of A at all, I will update. But just out of curiosity: is this a bug? The code worked for me, are there cases where it doesn't?

julia> A = rand(5) 5-element Array{Float64,1}: 0.819467 0.980514 0.190881 0.936337 0.882508 julia> for (i, j) in enumerate(A) @show A[j] end WARNING: Indexing with non-Integer Reals is deprecated. It may be that your index arose from an integer division of the form i/j, in which case you should consider using i÷j or div(i,j) instead. in depwarn at deprecated.jl:73 in to_index at deprecated.jl:447 in getindex at array.jl:282 [inlined code] from show.jl:127 in anonymous at no file:0 while loading no file, in expression starting on line 0 ERROR: InexactError() in to_index at deprecated.jl:448 in getindex at array.jl:282 [inlined code] from show.jl:127 in anonymous at no file:0 julia> for (i, a) in enumerate(A) @show a end a = 0.8194665233152139 a = 0.9805141257931855 a = 0.19088128894552314 a = 0.9363374114892509 a = 0.8825084643072016

for (i, j) in enumerate(A) is not @meggart wrote. He wrote for (i, j) in enumerate(eachindex(A)) which seems like it would work.

Oh, I was reading too fast. OK, then this wasn't a bug, just a style point.

tkelman · 2016-03-18T14:46:23Z

base/abstractarraymath.jl

@@ -179,8 +180,8 @@ end

 function ipermutedims(A::AbstractArray,perm)
    iperm = Array(Int,length(perm))
-    for i = 1:length(perm)
-        iperm[perm[i]] = i
+    for (i,j) = enumerate(eachindex(perm))


(i,p) = enumerate(perm) ?

meggart · 2016-03-21T13:26:26Z

Is this ok now?

timholy · 2016-03-21T14:42:09Z

LGTM. Maybe I should wait for someone else to take a second look 😉.

meggart · 2016-03-24T08:46:42Z

Another option would be to wait until #15459 gets merged and then revisit some of the FIXMEs.

timholy · 2016-03-24T15:47:59Z

Sorry for the delay here, @meggart. This is some nice work, exhibiting a clear understanding of the goals and strategies in #15434. I'm guessing #15459 is the beginning, but not end, of a thread of ideas, so I'd rather not hold this hostage to future improvements.

I see this got another thumbs up, so at least one set of eyeballs besides mine has checked this. To satisfy @KristofferC's excellent point in #15564 (comment), let's runbenchmarks("array", vs = "JuliaLang/julia:master").

musm · 2016-03-24T15:52:07Z

base/abstractarray.jl

@@ -1261,14 +1261,14 @@ end

 ## 2 argument
 function map!{F}(f::F, dest::AbstractArray, A::AbstractArray, B::AbstractArray)
-    for i = 1:length(A)
-        dest[i] = f(A[i], B[i])
+    for (i,j,k) = zip(eachindex(dest),eachindex(A),eachindex(B))


change to in to be consistent with the rest?

Also, spaces after commas are the most frequent convention.

pao · 2016-03-24T17:09:49Z

Something weird going on with nanosoldier? cc @jrevels

jrevels · 2016-03-24T17:46:32Z

It seems that the build failed due to some issue with threading. Do the other CI services not enable the JULIA_THREADS flag? I enable it for benchmark builds (if the Julia version is high enough) so that the threading benchmarks are available. I could disable that flag, if the problem is indeed unrelated to this PR.

Here's the last couple lines of the build log, if anybody's curious:

⋮
From worker 2:      CC src/signal-handling.o
From worker 2:      CC src/simplevector.o
From worker 2:      CC src/APInt-C.o
From worker 2:      CC src/runtime_intrinsics.o
From worker 2:      CC src/runtime_ccall.o
From worker 2:      CC src/threadgroup.o
From worker 2:      CC src/threading.o
/.../src/threading.c: In function ‘jl_threading_run’:
/.../src/threading.c:394:5: error: too many arguments to function ‘jl_get_specialization1’
jl_lambda_info_t *li = jl_get_specialization1(argtypes, NULL);
^
In file included from /.../src/threading.c:23:0:
/.../src/julia_internal.h:266:19: note: declared here
jl_lambda_info_t *jl_get_specialization1(jl_tupletype_t *types);
           ^
make[1]: *** [threading.o] Error 1
make[1]: *** Waiting for unfinished jobs....
In file included from /.../src/APInt-C.cpp:9:0:
/.../src/julia.h: In function ‘void jl_lock_frame_push(void (*)())’:
/.../src/julia.h:1374:32: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object [-Wpedantic]
locks->items[len] = (void*)unlock_func;
                        ^
/.../src/julia.h: In function ‘void jl_eh_restore_state(jl_handler_t*)’:
/.../src/julia.h:1388:47: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object [-Wpedantic]
     ((void(*)(void))locks->items[i - 1])();
                                       ^
In file included from /.../src/runtime_ccall.cpp:7:0:
/.../src/julia.h: In function ‘void jl_lock_frame_push(void (*)())’:
/.../src/julia.h:1374:32: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object [-Wpedantic]
locks->items[len] = (void*)unlock_func;
                        ^
/.../src/julia.h: In function ‘void jl_eh_restore_state(jl_handler_t*)’:
/.../src/julia.h:1388:47: warning: ISO C++ forbids casting between pointer-to-function and pointer-to-object [-Wpedantic]
     ((void(*)(void))locks->items[i - 1])();
                                       ^
make: *** [julia-src-release] Error 2

pao · 2016-03-24T18:09:19Z

It seems that the build failed due to some issue with threading.

#15608

timholy · 2016-03-24T18:15:10Z

I also have the impression that threading performance may have been broken for a while, so I'm not certain that the threading benchmarks are worth much now, unless one compares against a much older julia-0.5.

jrevels · 2016-03-24T18:16:52Z

I'll go ahead and disable the flag then. I'll retrigger the benchmarks here once I've made the change.

jrevels · 2016-03-24T18:24:02Z

runbenchmarks("array", vs = "JuliaLang/julia:master")

meggart · 2016-03-24T19:51:15Z

I updated lines 1039 and 1264 according to comments. Looks like this interrupted the perfomance tests? Sorry for this...

musm · 2016-03-24T20:26:05Z

base/arraymath.jl

@@ -362,8 +362,8 @@ function ctransposeblock!(B::AbstractMatrix,A::AbstractMatrix,m::Int,n::Int,offs
    return B
 end
 function ccopy!(B, A)
-    for i = 1:length(A)
-        B[i] = ctranspose(A[i])
+    for (i,j) = zip(eachindex(B),eachindex(A))


Also here use in

jrevels · 2016-03-24T21:04:41Z

. Looks like this interrupted the performance tests? Sorry for this...

Don't be! The performance tests are still running on the version of the PR that they were triggered on, it's just that GitHub's UI only shows the status of the latest version.

nanosoldier · 2016-03-25T03:33:08Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

timholy · 2016-03-25T09:13:06Z

Nice! @meggart, there often seem to be a few tests that get reported as regressions due to noise (there are hundreds of tests run, and so even a p < 0.01 statistical criterion would report a few false alarms). I would recommend checking the sum_each and possibly simd benchmark, but the other one looks like certain noise. The benchmarks are here. I don't think there's a convenient way to re-run specific cases (see https://github.com/JuliaCI/BenchmarkTrackers.jl/issues/16), so what I do is grep for the test and then copy/paste and run similar code locally.

If you need any help, give a shout!

KristofferC · 2016-03-25T11:39:23Z

The SIMD benchmarks are currently quite noisy.

timholy · 2016-03-25T14:22:09Z

I've definitely seen false reports on the sum_each benchmark too, so I'd give this a 95% chance of being fine. I'm therefore happy to merge this as-is, without further evidence that this doesn't hurt performance. Any objections?

timholy · 2016-03-26T10:31:04Z

Thanks very much, @meggart!

nalimilan reviewed Mar 14, 2016
View reviewed changes

meggart force-pushed the iteration branch from 1c4c937 to ebdd390 Compare March 14, 2016 16:07

tkelman reviewed Mar 15, 2016
View reviewed changes

meggart force-pushed the iteration branch from ebdd390 to f85cd93 Compare March 16, 2016 11:45

meggart force-pushed the iteration branch from f85cd93 to 75fdaf1 Compare March 17, 2016 09:30

musm reviewed Mar 17, 2016
View reviewed changes

meggart force-pushed the iteration branch from 75fdaf1 to ff34b15 Compare March 18, 2016 14:18

tkelman reviewed Mar 18, 2016
View reviewed changes

meggart force-pushed the iteration branch from ff34b15 to 9cae878 Compare March 18, 2016 14:53

musm reviewed Mar 24, 2016
View reviewed changes

fix for loop for array indexing

1901a9f

meggart force-pushed the iteration branch from 9cae878 to 1901a9f Compare March 24, 2016 19:49

musm reviewed Mar 24, 2016
View reviewed changes

timholy merged commit 6bf9e98 into JuliaLang:master Mar 26, 2016

fix for loop for array indexing #15497

fix for loop for array indexing #15497

Conversation

meggart commented Mar 14, 2016

Choose a reason for hiding this comment

nalimilan commented Mar 14, 2016

meggart commented Mar 14, 2016

IainNZ commented Mar 14, 2016

nalimilan commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meggart commented Mar 17, 2016

meggart commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meggart commented Mar 21, 2016

timholy commented Mar 21, 2016

meggart commented Mar 24, 2016

timholy commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pao commented Mar 24, 2016

jrevels commented Mar 24, 2016

pao commented Mar 24, 2016

timholy commented Mar 24, 2016

jrevels commented Mar 24, 2016

jrevels commented Mar 24, 2016

meggart commented Mar 24, 2016

Choose a reason for hiding this comment

jrevels commented Mar 24, 2016

nanosoldier commented Mar 25, 2016

timholy commented Mar 25, 2016

KristofferC commented Mar 25, 2016

timholy commented Mar 25, 2016

timholy commented Mar 26, 2016