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

Completely remove partial linear indexing #21750

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Completely remove partial linear indexing #21750

merged 2 commits into from
Aug 30, 2017

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented May 9, 2017

This removes the partial linear indexing deprecation and implents the new behavior: Linear indexing now only takes effect when there is exactly one non-cartesian index.

I know we've typically held off on removing deprecation until the mid-end of the release cycle, but this is a more complicated removal than typical, and several features that I'd like to see in this release depend on or are much simpler with this deprecation gone.

@mbauman mbauman added the domain:arrays [a, r, r, a, y, s] label May 9, 2017
@mbauman
Copy link
Sponsor Member Author

mbauman commented May 9, 2017

Hm, interesting. Tests pass in non-debug mode. In gdb on debug mode, I get:

#4  0x00007ffff75eeae2 in check_ambiguous_visitor (oldentry=0x7ffff13af2b0, 
    closure0=0x7fffffffc060)
    at /data/mbauman/julia6/src/gf.c:1129
1129        assert(!jl_subtype((jl_value_t*)sig, (jl_value_t*)type));
(gdb) p sig
$1 = (jl_tupletype_t *) 0x7fffeff3ba90
(gdb) p jl_(sig)
Tuple{typeof(Base.to_indices), Any, Tuple{}}
$2 = void
(gdb) p jl_(type)
Tuple{typeof(Base.to_indices), Any, Tuple{Vararg{Union{Integer, Base.IteratorsMD.CartesianIndex{N} where N}, N} where N}}
$3 = void

Any ideas?

@JeffBezanson
Copy link
Sponsor Member

Looks similar to #21710. I'll take a look.

@JeffBezanson
Copy link
Sponsor Member

#21761

@mbauman mbauman changed the base branch from master to jb/specificity21750 May 10, 2017 17:51
@mbauman mbauman changed the base branch from jb/specificity21750 to master May 10, 2017 17:52
JeffBezanson added a commit that referenced this pull request May 10, 2017
fix method specificity issue causing problems for pr #21750
tkelman pushed a commit that referenced this pull request May 14, 2017
@mbauman mbauman closed this May 23, 2017
@mbauman mbauman reopened this May 23, 2017
@timholy
Copy link
Sponsor Member

timholy commented May 23, 2017

LGTM.

@mbauman
Copy link
Sponsor Member Author

mbauman commented May 23, 2017

Just to make sure: @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@martinholters
Copy link
Member

So the benchmarks apparently make use of partial linear indexing:
https://github.com/JuliaCI/BaseBenchmarks.jl/blob/e3e89ca955ed6af8ba85fea44cb0e81ac0143677/src/array/sumindex.jl#L310-L311

@StefanKarpinski
Copy link
Sponsor Member

Bump.

This removes the partial linear indexing deprecation and implents the new behavior: Linear indexing now only takes effect when there is exactly one non-cartesian index.
mbauman added a commit to mbauman/BaseBenchmarks.jl that referenced this pull request Aug 24, 2017
@mbauman
Copy link
Sponsor Member Author

mbauman commented Aug 24, 2017

Looking through recent BaseBenchmarkReports, it looks like JuliaCI/BaseBenchmarks.jl#106 will be sufficient to fix up the Nanosoldier with this branch.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Aug 24, 2017

Hm, this is ending up with a segfault in one of the codegen tests… I trapped it with gdb but all I can see is that it's ending up trying to access null pagetable0_t somewhere in the GC structures. I'll have to dig into this further next week.

@mbauman
Copy link
Sponsor Member Author

mbauman commented Aug 28, 2017

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

@ararslan
Copy link
Member

Hmmmm. @nanosoldier runbenchmarks(ALL, vs=":master")

@mbauman
Copy link
Sponsor Member Author

mbauman commented Aug 28, 2017

Yeah, those failures are still expected. I forgot that this depended upon JuliaCI/BaseBenchmarks.jl#106.

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

ararslan pushed a commit to JuliaCI/BaseBenchmarks.jl that referenced this pull request Aug 28, 2017
* Remove dependence on partial linear indexing

Needed to test performance of JuliaLang/julia#21750.

* Bump Compat dependency to 0.31.0
@ararslan
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

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

@mbauman
Copy link
Sponsor Member Author

mbauman commented Aug 29, 2017

Thanks so much for shepherding those changes into Nanosoldier, @ararslan! This is looking pretty good now. The FreeBSD failure is in a random fuzzing block within test/ranges.jl — have we seen that before? I managed to reproduce it locally on a different branch by increasing the number of iterations in that loop. I'll try to narrow it down and open an issue.

I'd like to merge this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants