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

Ensure n-ary to_index inlines #11833

Merged
merged 3 commits into from
Jun 28, 2015
Merged

Ensure n-ary to_index inlines #11833

merged 3 commits into from
Jun 28, 2015

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jun 23, 2015

While I had fixed the type instability of to_index in 6a3c173, it stopped inlining even sooner!

This is the right solution — use a recursive n-ary splatted argument list. This fixes #11819 properly. I don't have time to run the test suite locally, but assuming CI passes this is good to go.

mbauman added a commit that referenced this pull request Jun 24, 2015
Bootstrap seems to require getindex/setindex! to be defined for ::Real, just for Win64.  This will be able to be simplified later with #11833.

Triaged together with @tkelman. Tests pass on OS X 64 and Win64 locally.
@mbauman mbauman mentioned this pull request Jun 24, 2015
@mbauman
Copy link
Member Author

mbauman commented Jun 25, 2015

Argh, it seems Win64 still doesn't like this simplification to the Array getindex methods.

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

The win64 failure was the timeout that's been happening everywhere (and the latest commit here more-or-less worked for me locally), but I didn't try locally on win32 yet and there's apparently an assertion failure in llvm

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.
Assertion failed!
Program: C:\projects\julia\usr\bin\julia.exe
File: /home/Tony/julia32/deps/llvm-3.3/include/llvm/ADT/SmallVector.h, Line 144
Expression: begin() + idx < end()
    From worker 2:       * math                Worker 2 terminated.

@mbauman mbauman force-pushed the mb/to_index branch 2 times, most recently from 7339da2 to 8332ad6 Compare June 27, 2015 03:41
@mbauman
Copy link
Member Author

mbauman commented Jun 27, 2015

Well, lets see if it happens again. I squashed and added a deprecation (since a few packages had been using this and it was easy to deprecate).

@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

(initially posted this in the wrong issue, don't triage before coffee)

looks like the win32 problem is legitimate, though different error this time?

exception on 2: ERROR: LoadError: test failed: !(isequal(s.dict.keys,k))
 in expression: !(isequal(s.dict.keys,k))
 in error at error.jl:21
 in default_handler at test.jl:29
 in do_test at test.jl:52
 in include at boot.jl:254
 in runtests at C:\projects\julia\test\testdefs.jl:197
 in anonymous at multi.jl:837
 in run_work_thunk at multi.jl:590
 in anonymous at task.jl:837
while loading C:\projects\julia\test\sets.jl, in expression starting on line 95
    From worker 3:       * replutil             in  10.43 seconds
ERROR: LoadError: LoadError: test failed: !(isequal(s.dict.keys,k))
 in expression: !(isequal(s.dict.keys,k))
 in anonymous at task.jl:1403
while loading C:\projects\julia\test\sets.jl, in expression starting on line 95
while loading C:\projects\julia\test\runtests.jl, in expression starting on line 5

    From worker 2:       * sets                Command exited with code 1

edit: this failure is unrelated, #11890

This separates the n-ary version of to_index to to_indexes. Recursive definitions with tuples don't inline like recursive splatted arguments.
@mbauman mbauman force-pushed the mb/to_index branch 2 times, most recently from 6286196 to d01e25d Compare June 27, 2015 15:40
mbauman added 2 commits June 27, 2015 16:14
While this was an internal function, it was used in at least a few other packages.  It is easy to deprecate, so we may as well do so.
mbauman added a commit that referenced this pull request Jun 28, 2015
@mbauman mbauman merged commit 1685834 into master Jun 28, 2015
@mbauman mbauman deleted the mb/to_index branch June 28, 2015 03:40
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.

Performance regression when indexing an 4+ dimensional array
2 participants