Skip to content

Style fixes for Bridges#1344

Merged
odow merged 4 commits intomasterfrom
od/bridge-style
May 13, 2021
Merged

Style fixes for Bridges#1344
odow merged 4 commits intomasterfrom
od/bridge-style

Conversation

@odow
Copy link
Copy Markdown
Member

@odow odow commented May 13, 2021

Style fixes and unification of return types for a small win. This was mainly an excuse for me to read slowly through the code to better understand it.

Before

Running: clp 
 10.690129 seconds (25.79 M allocations: 1.501 GiB, 6.46% gc time, 21.90% compilation time)
  0.001109 seconds (1.58 k allocations: 114.789 KiB)

After

Running: clp 
 10.350866 seconds (25.08 M allocations: 1.463 GiB, 6.01% gc time, 21.59% compilation time)
  0.000871 seconds (1.58 k allocations: 114.789 KiB)

Edit: I'll add that functions who didn't explicitly return could have type unstable return types at inference. At run time these were obviously never used, which explains the identical second-time results. It's only an issue on the first run when inference has to attempt to figure out what the return is. A blank return lets it figure this out easily.

@odow odow added Submodule: Bridges About the Bridges submodule Type: Performance labels May 13, 2021
@odow
Copy link
Copy Markdown
Member Author

odow commented May 13, 2021

Okay, I now understand the bridging code a lot better.

This PR is only style and returns. No logic or bigger code changes, so it should be pretty safe to merge.

  • I added some extra TODOs to the code, and I noticed a few scattered around. Once this is merged, I'll open an issue to track them all.
  • Some bridges return a vector for things like ListOfVariableIndices that they use internally. Since we have the (unstated???) contract that results returned from MOI.get are now the property of the caller, these should switch to returning copies. Anything else just seems liable to introduce bugs.
  • There is a bit of duplication of code and types, e.g., IndexInVector and trimap. We should simplify these, where possible.

The core reason for the time-to-first-plot problem is this:

  • We have a lot of code in the bridges. This code, by design is not type-stable, since it uses things like a list of added bridges. A lot of the core reasoning for doing this was that Julia could figure out which bridges were used "at compile time", but since we produce code that doesn't compile well, a lot of this happens at run-time. I wonder if we need something similar to Lazily create storage in StructOfConstraints #1315, where the .variable_map, .constraint_map, and .objetive_map fields are Union{Nothing to give the compiler a better chance and cut the amount of code it tries to infer. This might be the equivalent of Add auto_bridge to CachingOptimizer #1252.

We at least have some threads to pull on going forward.

@odow odow merged commit 7923c60 into master May 13, 2021
@odow odow deleted the od/bridge-style branch May 13, 2021 20:54
@blegat blegat added this to the v0.10 milestone May 22, 2021
blegat pushed a commit that referenced this pull request May 22, 2021
blegat pushed a commit that referenced this pull request May 22, 2021
blegat pushed a commit that referenced this pull request May 23, 2021
@blegat blegat modified the milestones: v0.10, v0.9.22 May 25, 2021
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.

2 participants