Skip to content

Conversation

joaquimg
Copy link
Member

Improves:

  • dense dict type stability
  • indexmap copying and reversing in attach optimizer

cc @blegat

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Benchmarks?

@joaquimg
Copy link
Member Author

Benchmarks?

jump-dev/MOIPaperBenchmarks#3

end
end

function inverse_hash(d::DenseDict{K, V, F, I}, el)::K where {K,V,F,I}
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed with _index_to_variable ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed not needed anymore

else
el, i = itr
return d.inverse_hash(el) => d.map[el], i
return d.inverse_hash(el)::K => d.map[el]::V, i
Copy link
Member

Choose a reason for hiding this comment

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

Are the ::K and ::V needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tested, Julia does not seem to infer those correctly. Therefore the is a very minor difference a little less than 1%.
I vote for leaving it there, since its not restricting anything.
However, if we really want to avoid that, I can remove.

Copy link
Member

Choose a reason for hiding this comment

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

No if Julia inference is not doing it correctly then leaving it there is what we should do

@blegat
Copy link
Member

blegat commented Jul 28, 2020

This iline:

model.optimizer_to_model_map[indexmap[k]] = k

is taking the majority of time for a benchmark with jump-dev/SCS.jl#192
indexmap_copy
We might want to set this optimizer_to_index_map to nothing and only create it when needed

@joaquimg
Copy link
Member Author

This line:

Did you run with this PR?
This line is not there anymore.

@blegat
Copy link
Member

blegat commented Jul 28, 2020

No, I run with master, my point is that this PR is needed

@joaquimg joaquimg mentioned this pull request Aug 6, 2020
@joaquimg
Copy link
Member Author

joaquimg commented Aug 6, 2020

@blegat ready to merge this?
I had to revert a commit, so it would be better to squash when merging.

@blegat blegat merged commit 746b2e1 into master Aug 6, 2020
@odow odow deleted the jg/indexmap_perf branch September 2, 2020 21:23
@blegat blegat added this to the v0.9.15 milestone Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants