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.

Sign up for GitHub

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

Jump to bottom

Avoid inlining setindex!(::Array, ::Any, ::AbstractVector) #21401

Closed
wants to merge 1 commit into from
Closed

Avoid inlining setindex!(::Array, ::Any, ::AbstractVector) #21401

wants to merge 1 commit into from

Conversation

nalimilan
Copy link
Member

@_propagate_inbounds_meta forces inlining, which may not be desirable
for such complex functions. To avoid this, define a wrapper which will be
inlined, and which will call either the safe or the unsafe method.

Follows a suggestion by @simonster at #20910 (comment). Unfortunately, this makes the code significantly more complex; maybe there's a way to make this slightly cleaner.

FWIW, it turns out that the compiler decides to inline _unsafe_setindex! when @inbounds is passed, but _safe_setindex! remains as a function call.

@nalimilan
Avoiding inlining setindex!(::Array, ::Any, ::AbstractVector)
129d786 
@_propagate_inbounds_meta forces inlining, which may not be desirable
for such complex functions. To avoid this, define a wrapper which will be
inlined, and which will call either the safe or the unsafe method.
@nalimilan nalimilan requested review from mbauman and simonster April 16, 2017 12:52
@JeffBezanson
Copy link
Member

What is the impact of this change?

@nalimilan
Copy link
Member Author

What is the impact of this change?

In terms of? The idea is to avoid always inlining setindex!, which was the case since #20910.

@nalimilan nalimilan changed the title Avoiding inlining setindex!(::Array, ::Any, ::AbstractVector) Avoid inlining setindex!(::Array, ::Any, ::AbstractVector) Apr 16, 2017
@JeffBezanson
Copy link
Member

I mean, why do I care? What is the benefit?

@nalimilan
Copy link
Member Author

Well, just like for any relatively large function, inlining makes the code bigger and it's not necessarily a win in terms of performance, so better leave the compiler heuristics decide what to do. I guess you'd better ask @simonster. :-)

@nalimilan
Copy link
Member Author

The CI failure is quite obscure and comes from BitArray. All tests pass, but the error happens in make docs. It can be reproduced with:

x = ["a"]
x[.!trues(1)]

This in turns comes from the fact that find(.!trues(1)) unexpectedly returns fill(0, 32).

@nalimilan
Copy link
Member Author

I've tracked down the failure to the _broadcast! specialization for BitArray, and most probably to the dumpbitcache function it calls. This is very likely due to this known issue: #20065 (comment)

@nalimilan nalimilan mentioned this pull request Apr 16, 2017
Closed
@mbauman
Copy link
Member

mbauman commented Apr 17, 2017

This generally seems like the right thing to do — we do something similar for the generic versions in multidimensional.jl. It's a little unfortunate that it's so complicated and is an exact duplication of that functionality, but I imagine the comment about it being required for bootstrap is still true.

@mbauman
Copy link
Member

mbauman commented Apr 17, 2017 •
edited
Loading

Interestingly, I cannot reproduce that failure on a 9-day old master (a17b1ae on westmere & macOS). (Edit: although maybe it depends on uninitialized memory being a particular kind of dirty.) Not interesting; of course it requires this patch to be applied.

@nalimilan
Copy link
Member Author

It's a little unfortunate that it's so complicated and is an exact duplication of that functionality, but I imagine the comment about it being required for bootstrap is still true.

I've just tried removing them, and indeed bootstrap fails.

Interestingly, I cannot reproduce that failure on a 9-day old master (a17b1ae on westmere & macOS).

You mean, with the patch from this PR applied? Else it doesn't happen. For some reason, this PR triggers the existing bug in more cases than previously.

@mbauman
Copy link
Member

mbauman commented Apr 17, 2017

You mean, with the patch from this PR applied? Else it doesn't happen. For some reason, this PR triggers the existing bug in more cases than previously.

Doh, of course. Sorry for the noise.

@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 @jrevels

@mbauman
Copy link
Member

mbauman commented Jul 13, 2017

Is this still relevant after #22210?

@nalimilan
Copy link
Member Author

I guess so, since this PR is about avoiding forced inlining, which will allow the compiler to decide whether it's best to inline or not.

@timholy
Copy link
Member

timholy commented Jul 13, 2017 •
edited
Loading

#22210 didn't, in the end, remove any of the @inline annotations in Base. Benchmarks suggest that the vast majority could now be removed, however. I don't have time to tackle this myself, but if someone wants to try this the approach I'd recommend is:

  • locally build two separate installations of Julia, one for reference (master) and one for the PR
  • disable the @inline macros in your PR branch (you can rebase branch teh/disable_inline_macros)
  • run the benchmark suite on both branches and then look for regressions, see https://github.com/JuliaCI/BaseBenchmarks.jl
  • go through and delete all the inline annotations from things that don't show up as regressions, re-enable the @inline macros
  • rerun the Benchmarks and resolve any remaining regressions

@rfourquet rfourquet mentioned this pull request Sep 27, 2017
Merged
@DilumAluthge DilumAluthge deleted the nl/setindex! branch August 24, 2021 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman Awaiting requested review from mbauman

@simonster simonster Awaiting requested review from simonster

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nalimilan @JeffBezanson @mbauman @ararslan @nanosoldier @timholy @oscardssmith