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

[FileFormats.MOF] replace OrderedDict by NamedTuple when writing #2606

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Conversation

joaquimg
Copy link
Member

@joaquimg joaquimg commented Jan 9, 2025

All OrderedDicts used are very small. Hence, they allocate unnecessarily. This also creates more GC pressure.

@odow
Copy link
Member

odow commented Jan 9, 2025

Could we get some benchmarks?

src/FileFormats/MOF/write.jl Outdated Show resolved Hide resolved
src/FileFormats/MOF/write.jl Outdated Show resolved Hide resolved
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.

This is quite nice

@odow odow changed the title [FileFormats] MOF write performance - replace OrderedDict by NamedTuple [FileFormats.MOF] replace OrderedDict by NamedTuple when writing Jan 9, 2025
src/FileFormats/MOF/write.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Jan 9, 2025

I opened an issue in JuliaFormatter to discuss formatting the named tuples: domluna/JuliaFormatter.jl#895

@joaquimg
Copy link
Member Author

joaquimg commented Jan 9, 2025

A test

using JuMP

model = Model()
@variable(model, x[1:1000] >= 0)
@constraint(model, y[1:1000], sum(i*x[i] for i in 1:1000) == 1)

for i in 1:5
    GC.gc()
    @time write_to_file(model, "test.mof.json")
end

Results:

before PR:

  5.528170 seconds (20.16 M allocations: 1.669 GiB, 31.54% gc time)
  5.244265 seconds (20.16 M allocations: 1.669 GiB, 30.18% gc time)
  5.487519 seconds (20.16 M allocations: 1.669 GiB, 29.10% gc time)
  5.344851 seconds (20.16 M allocations: 1.669 GiB, 30.58% gc time)
  5.348303 seconds (20.16 M allocations: 1.669 GiB, 29.13% gc time)

after PR:

  2.293431 seconds (5.13 M allocations: 936.505 MiB, 13.93% gc time)
  2.281071 seconds (5.13 M allocations: 936.508 MiB, 13.94% gc time)
  2.995470 seconds (5.13 M allocations: 936.454 MiB, 12.53% gc time)
  3.088288 seconds (5.13 M allocations: 936.497 MiB, 12.26% gc time)
  2.971376 seconds (5.13 M allocations: 936.498 MiB, 12.24% gc time)

Which is a big win as there is also the fixed cost of writing the json that both incur.

@odow
Copy link
Member

odow commented Jan 9, 2025

Nice nice nice.

Note that this could be regarded as technically breaking, but I don't think anyone is using moi_to_object: https://juliahub.com/ui/Search?type=code&q=moi_to_object. It's also not mentioned in the docs, so I'm going to say that it is the private API.

@odow
Copy link
Member

odow commented Jan 9, 2025

Reduction in memory pressure is a big deal for the larger Sienna models.

cc @jd-lara this should help your 'always write to MOF'

@joaquimg
Copy link
Member Author

joaquimg commented Jan 9, 2025

should we rename moi_to_object to _moi_to_object?

@odow
Copy link
Member

odow commented Jan 9, 2025 via email

@joaquimg
Copy link
Member Author

joaquimg commented Jan 9, 2025

green light from CI

"coefficient" => foo.coefficient,
"variable" => name_map[foo.variable],
)
return (coefficient = foo.coefficient, variable = name_map[foo.variable])
Copy link
Member

Choose a reason for hiding this comment

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

This should make such a difference; we were making a new OrderedDict for every single coefficient in the model 😭

@odow odow merged commit 439d263 into master Jan 10, 2025
16 checks passed
@odow odow deleted the jg/mof branch January 10, 2025 01:11
@odow
Copy link
Member

odow commented Jan 10, 2025

@joaquimg do you have a script for the before/after Sienna profile flame graph?

@joaquimg
Copy link
Member Author

Is the same script used here (but with PProf):

NREL-Sienna/PowerSimulations.jl#1183

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