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

Byte threshold for ConstantFolding transformation #25239

Closed

Conversation

itikhono
Copy link
Contributor

Details:

Adding a byte threshold for ConstantFolding transformation.
If byte size of the folded constant exceed this threshold, we do not fold this operation.

Tickets:

@itikhono itikhono requested review from a team as code owners June 26, 2024 19:01
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: transformations OpenVINO Runtime library - Transformations category: CPP API OpenVINO CPP API bindings labels Jun 26, 2024
@@ -125,6 +125,7 @@ bool ov::pass::MOCTransformations::run_on_model(const std::shared_ptr<ov::Model>
f->validate_nodes_and_infer_types();
}

const int64_t cf_threshold = 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you give explanation what is threshold? How is this value selected?
Also, I think it should be a macro, sort of configuration for ConstantFolding

Copy link
Contributor Author

@itikhono itikhono Jun 27, 2024

Choose a reason for hiding this comment

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

1024 is just a preliminary estimation: usually ConstantFolding folds 3-4 operations (~250 bytes each) from xml file during one step, so to make the sum of IR xml + bin sizes more or less stable, we allow ConstantFolding to increase the bin size to this value.

ok, I will add the macro

// do not apply ConstantFolding for this particular node because
// the created Constant might increase the bin size beyond the required threshold.
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clearify. CF_BYTE_THRESHOLD - is difference for entire model or for particular node? I see current calculation is doing for each node.

// So ~1000 bytes will be deleted from xml file.
// To make the sum of xml + bin more or less stable,
// we allow ConstantFolding to increase the bin size to this value.
#define CF_BYTE_THRESHOLD 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define CF_BYTE_THRESHOLD 1024
constexpr size_t CF_BYTE_THRESHOLD 1024

?

private:
// this variable controls the threshold by which it is allowed to increase
// the size of the binary file during one step of the ConstantFolding execution.
int64_t m_byte_threshold;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int64_t m_byte_threshold;
const size_t m_byte_threshold;

Should we allow change after creation (remove const is in most cases easier).
Set it to max as default and use it if less then max.

for (const auto& input_value : node->input_values()) {
auto constant = ov::as_type_ptr<ov::op::v0::Constant>(input_value.get_node_shared_ptr());
if (constant && is_threshold_applicable) {
byte_size_before += static_cast<int64_t>(constant->get_byte_size());
Copy link
Contributor

Choose a reason for hiding this comment

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

If threshold would be unsigned then this byte size also can be and cast will be not required.

if (constant && is_threshold_applicable) {
byte_size_before += static_cast<int64_t>(constant->get_byte_size());
} else {
is_threshold_applicable = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_threshold_applicable = false;
is_threshold_applicable = false;
break;

?

byte_size_after += static_cast<int64_t>(
element::get_memory_size(output.get_element_type(), ov::shape_size(output.get_shape())));
} else {
is_threshold_applicable = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_threshold_applicable = false;
is_threshold_applicable = false;
break;

?

broadcast_v1->set_friendly_name("test");
auto f = make_shared<Model>(broadcast_v1, ParameterVector{});

run_constant_folding_with_threshold(f, 1024);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run_constant_folding_with_threshold(f, 1024);
run_constant_folding_with_threshold(f, CF_BYTE_THRESHOLD );

Copy link
Contributor

github-actions bot commented Aug 4, 2024

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Aug 4, 2024
Copy link
Contributor

This PR was closed because it has been stalled for 2 week with no activity.

@github-actions github-actions bot closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: transformations OpenVINO Runtime library - Transformations do_not_merge Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants