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

Update memory resource interfaces. #2742

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Update memory resource interfaces. #2742

merged 2 commits into from
Mar 1, 2021

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Mar 1, 2021

Signed-off-by: Michał Zientkiewicz [email protected]

Why we need this PR?

Pick one, remove the rest

  • Refactoring to match proposed libcu++ memory resource interfaces.

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    • Some renaming
    • Change allocation_order enum to Context type parameter
  • Affected modules and functionalities:
    • Memory manager, third_party
  • Key points relevant for the review:
    • N/A
  • Validation and testing:
    • Existing tests apply
  • Documentation (including examples):
    • N/A

JIRA TASK: N/A

Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient mzient requested a review from a team March 1, 2021 14:14
Signed-off-by: Michał Zientkiewicz <[email protected]>
@mzient
Copy link
Contributor Author

mzient commented Mar 1, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119321]: BUILD STARTED

@JanuszL JanuszL self-assigned this Mar 1, 2021
@@ -163,6 +163,10 @@ class test_resource_wrapper<owning, security_check, memory_resource<kind, order>
return this->upstream_->deallocate(p, b, a);
}, ptr, bytes, alignment);
}

virtual Context do_get_context() const noexcept {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cannot be marked override because the base class for any_context does not contain a virtual function of this name (for any context this function is never used, but it's here regardless to avoid excessive complexity).

Copy link
Contributor

Choose a reason for hiding this comment

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

Every time I see a comment in review that justifies how the code is written I think it should probably be put into the code itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot agree more.

Copy link
Contributor Author

@mzient mzient Mar 1, 2021

Choose a reason for hiding this comment

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

Explains - yes.
Justifies - not really.
That would be just noise and it's likely to go stale should this function make its way back to the interface (which I'm arguing for).

@@ -163,11 +163,15 @@ class monotonic_memory_resource<kind, order, true> : public memory_resource<kind
void do_deallocate(void *data, size_t bytes, size_t alignment) override {
}

virtual Context do_get_context() const noexcept {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot mark override due to variable interface - see previous comment.

@@ -254,9 +258,13 @@ class monotonic_memory_resource<kind, order, false> : public memory_resource<kin
void do_deallocate(void *data, size_t bytes, size_t alignment) override {
}

virtual Context do_get_context() const noexcept {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot mark override due to variable interface - see previous comment.

@@ -127,14 +127,18 @@ class pool_resource_base : public memory_resource<kind, order> {
}
}

virtual Context do_get_context() const noexcept {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot mark override due to variable interface - see previous comment.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119342]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119402]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119402]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119342]: BUILD PASSED

@jantonguirao jantonguirao self-assigned this Mar 1, 2021
@mzient mzient merged commit 746f6df into NVIDIA:master Mar 1, 2021
@JanuszL JanuszL mentioned this pull request May 19, 2021
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.

5 participants