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

small optimization to f(...; kw...) when kw is empty #21601

Merged
merged 1 commit into from
May 1, 2017

Conversation

JeffBezanson
Copy link
Member

Found a modest piece of low-hanging fruit here. In this case, we were "copying" the container kw to a new (empty) array, and then checking whether that is empty, when we could just check whether kw is empty. This comes up a lot in definitions like

call(f, args...; kwargs...) = f(args...; kwargs...)

(e.g. remotecall), where allocating an unnecessary empty array has significant overhead for what should be a no-op. This shaves off another ~10% of the memory allocated in the benchmark in #21543.

This also takes advantage of some common cases where kw has a known length, where before we always copied it using push!.

@JeffBezanson JeffBezanson added the performance Must go faster label Apr 27, 2017
@JeffBezanson
Copy link
Member Author

Ha, that failure was a good puzzle. Apparently @testset depended on the (unnecessary) for loop generated by keyword arg lowering to force toplevel expressions to be compiled, in order to get better backtraces.

@@ -848,6 +848,9 @@ function testset_beginend(args, tests)
# action (such as reporting the results)
quote
ts = $(testsettype)($desc; $options...)
# this empty loop is here to force the block to be compiled,
# which is needed for backtrace scrubbing to work correctly.
while false; end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem fragile at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, suggest an alternative. There is a test for this at least.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would putting it in a let work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine. If we change the front-end heuristics, we'll worry about this then. Probably by then backtrace scrubbing will be decoupled from compilation anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe put a comment on whichever test led you to notice this, as a breadcrumb for the next person who runs into this?

@tkelman
Copy link
Contributor

tkelman commented Apr 28, 2017

@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

@Sacha0
Copy link
Member

Sacha0 commented Apr 28, 2017

Chances are the potential regressions are noise. But it's dangerous to go alone, so take this tiny metal pegasus @nanosoldier runbenchmarks("array" || "sparse" || "tuple", 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

@Sacha0
Copy link
Member

Sacha0 commented Apr 28, 2017

The potential ["tuple","reduction",("sumabs",(4,))] and ["sparse","transpose",("ctranspose!",(20000,10000))] regressions are consistent across nanosoldier runs, but those two benchmarks are perennially noisy so I'm not certain what to make of that consistency...

@JeffBezanson
Copy link
Member Author

They're consistently inconsistent :)

@JeffBezanson
Copy link
Member Author

JeffBezanson commented Apr 28, 2017

Interesting failure on os x. Log backed up:
https://gist.github.com/JeffBezanson/638cba8d73c1b46963e4971135db70ab

@JeffBezanson JeffBezanson merged commit 41757ef into master May 1, 2017
@JeffBezanson JeffBezanson deleted the jb/faster_empty_kw branch May 1, 2017 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants