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

warnings for some common performance traps #480

Merged
merged 1 commit into from
Jul 15, 2015
Merged

Conversation

mlubin
Copy link
Member

@mlubin mlubin commented Jul 14, 2015

This covers some of the common cases but not everything. I think we should wait for user feedback before expanding the scope of performance warnings like this.

@mlubin mlubin force-pushed the performance_warn branch 2 times, most recently from 3c753c0 to d72d9d0 Compare July 15, 2015 01:51
@mlubin mlubin force-pushed the performance_warn branch from d72d9d0 to 34742ee Compare July 15, 2015 01:52
@mlubin
Copy link
Member Author

mlubin commented Jul 15, 2015

Added void returns to avoid unnecessary allocations, otherwise don't see any performance regressions

mlubin added a commit that referenced this pull request Jul 15, 2015
warnings for some common performance traps
@mlubin mlubin merged commit 711f4f2 into master Jul 15, 2015
@mlubin mlubin deleted the performance_warn branch July 15, 2015 02:07
@joehuchette
Copy link
Contributor

This doesn't play nicely with the vectorized stuff, which uses this fallback.

using JuMP

A = rand(250,250)
B = rand(250,250)
b = rand(250)

m = Model()
@defVar(m, x[1:250])

@addConstraint(m, A*x + B*x .<= b) # gives warning

@mlubin
Copy link
Member Author

mlubin commented Jul 22, 2015

Hmm, any suggestions on how to improve the warning? Bump the threshold for operator_counter to 10000?

@joehuchette
Copy link
Contributor

Actually I think the real problem should be in the _multiply! functions this calls, let me play around with it.

@joehuchette
Copy link
Contributor

But yeah, bumping it up seems prudent as well.

@joehuchette
Copy link
Contributor

This particular instance is fixed with dac3402

@mlubin
Copy link
Member Author

mlubin commented Jul 22, 2015

At least the warnings help us find cases where we're doing too much operator overloading.

@joehuchette
Copy link
Contributor

The example above still gives warnings on v0.3

@joehuchette
Copy link
Contributor

(because @addConstraint generates different code on v0.3 and v0.4)

@mlubin
Copy link
Member Author

mlubin commented Jul 28, 2015

Hmm. We could disable the performance warnings on 0.3.

@joehuchette
Copy link
Contributor

I'm gonna see if I can do something clever here, but otherwise yeah, let's do that and bump up the counter too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants