Skip to content

Rework parts of Graph::Resolve to reduce memory usage#12176

Merged
skottmckay merged 6 commits intomasterfrom
skottmckay/ReduceGraphResolveMemoryCost
Aug 5, 2022
Merged

Rework parts of Graph::Resolve to reduce memory usage#12176
skottmckay merged 6 commits intomasterfrom
skottmckay/ReduceGraphResolveMemoryCost

Conversation

@skottmckay
Copy link
Contributor

@skottmckay skottmckay commented Jul 14, 2022

Description:

  • Remove manual list of outer scope values that were used by BuildConnections. The information can be derived from other data structures that are initialized prior to BuildConnections being called.
  • Use string_view instead of string in ResolveContext to reduce copies
  • Remove recursion in VerifyNodeAndOpMatch. It's called via the subgraph type/shape inferencing so will recurse into subgraphs via that.
  • Clear ResolveContext as soon as Graph::Resolve is done to reduce the period it is consuming memory (was previously cleared at the start of Graph::Resolve), and prevent any potential issues from usage of string_view in it (underlying string could be in container that changes during other Graph modifications, rendering the string_view invalid).

Motivation and Context
Reduce memory consumption for models that have a lot of subgraphs, especially if there are multiple levels of nesting.

Saves 7MB of peak memory loading 1P model.

@skottmckay skottmckay marked this pull request as draft July 14, 2022 10:40
@skottmckay skottmckay marked this pull request as ready for review July 29, 2022 00:04
@skottmckay skottmckay requested a review from hariharans29 July 29, 2022 00:05
@skottmckay skottmckay requested a review from edgchen1 August 4, 2022 05:47
std::unordered_map<std::string_view, NodeIndex> node_name_to_index;
std::unordered_set<Node*> nodes_with_subgraphs;

// check if the provided name is an input/initialize/node output of this Graph instance during Graph::Resolve.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Initializer ?

std::unordered_map<std::string, NodeIndex> node_name_to_index;
std::unordered_map<std::string_view, std::pair<Node*, int>> output_args;
std::unordered_set<std::string_view> inputs_and_initializers;
std::unordered_map<std::string_view, NodeIndex> node_name_to_index;
Copy link
Member

@hariharans29 hariharans29 Aug 5, 2022

Choose a reason for hiding this comment

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

Since we are touching these anyway, any chance we want to start using InlinedHashMaps here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the usage of those are small collections where we're trying to avoid a memory allocation by having a small stack allocated buffer. These collections will most likely be quite large so are very unlikely to be able to use the stack allocated buffer.

@skottmckay skottmckay merged commit 8d830ad into master Aug 5, 2022
@skottmckay skottmckay deleted the skottmckay/ReduceGraphResolveMemoryCost branch August 5, 2022 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants