-
Notifications
You must be signed in to change notification settings - Fork 87
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
[FileFormats.MPS] improve performance of MPS writer #2418
Conversation
Now to julia> GC.gc(); @time main(100_000);
1.362330 seconds (10.25 M allocations: 526.213 MiB, 9.37% gc time) |
Now to julia> GC.gc(); @time main(100_000);
1.184845 seconds (8.85 M allocations: 475.188 MiB, 11.24% gc time) |
now to
|
Now to julia> GC.gc(); @time main(100_000);
1.116765 seconds (6.90 M allocations: 436.318 MiB, 11.98% gc time) |
fdf3223
to
92f4637
Compare
Miles' example is now:
Original was
So nearly 1/3 the allocations and 60% of the total allocated. I've been mucking around with storing the indices instead of names, but yet to find something that makes a difference. |
Now
|
now
|
I tried replacing the |
Nice improvements! |
To do better we'd need a much larger rewrite that stored the problem in
column order to start with instead of the current constraint based
approach. You probably still have a 2X memory penalty as we transpose the
matrix.
…On Tue, 6 Feb 2024, 2:01 pm Miles Lubin, ***@***.***> wrote:
Nice improvements!
—
Reply to this email directly, view it on GitHub
<#2418 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB6MQJMRAEIF35ZII3C65XTYSF6GFAVCNFSM6AAAAABCZFSEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGU4DMNZQG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Note to self. But we should also benchmark writing direct from HiGHS or
Gurobi.
…On Tue, 6 Feb 2024, 3:37 pm Oscar Dowson, ***@***.***> wrote:
To do better we'd need a much larger rewrite that stored the problem in
column order to start with instead of the current constraint based
approach. You probably still have a 2X memory penalty as we transpose the
matrix.
On Tue, 6 Feb 2024, 2:01 pm Miles Lubin, ***@***.***> wrote:
> Nice improvements!
>
> —
> Reply to this email directly, view it on GitHub
> <#2418 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB6MQJMRAEIF35ZII3C65XTYSF6GFAVCNFSM6AAAAABCZFSEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGU4DMNZQG4>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (didn't review the code in detail)
Merging this as an improvement for now. But I have a plan for a larger change. |
This sounds related to what |
MatrixOfConstraints can't incrementally build a CSC though right? It just does it in a The main issue with the current design is that when writing, you can the CSC, and when reading, you want the CSR. At the moment, we use a single data structure, but it'd be nice to switch between them depending on whether you're reading or writing. |
No, it's done incrementally. |
Using the
Optimizer
from #2417Before:
After:
Small improvement in time, but big improvement in the number of allocations.