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.

Sign up for GitHub

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

Jump to bottom

new method to manage resource in engine #194

Merged
merged 1 commit into from
Dec 11, 2024
Merged

new method to manage resource in engine #194

merged 1 commit into from
Dec 11, 2024

Conversation

LHT129
Copy link
Collaborator

@LHT129 LHT129 commented Dec 9, 2024 •
edited
Loading

issue: #203

@LHT129 LHT129 added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Dec 9, 2024
@LHT129 LHT129 self-assigned this Dec 9, 2024
@LHT129 LHT129 force-pushed the factory branch 5 times, most recently from c4675c4 to fcac7fd Compare December 10, 2024 07:12
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 10, 2024
@LHT129 LHT129 force-pushed the factory branch 5 times, most recently from 64e4e5f to 67a2e71 Compare December 10, 2024 11:39
@LHT129 LHT129 changed the title introduce allocator for factory new method to manage resource in engine Dec 10, 2024
wxyucs
wxyucs reviewed Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@wxyucs wxyucs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@LHT129 please add an example of this change, and also need create an issue about this feature

@wxyucs wxyucs added kind/feature New feature or request and removed kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) labels Dec 10, 2024
jiaweizone
jiaweizone reviewed Dec 10, 2024
View reviewed changes
include/vsag/engine.h Outdated
explicit Engine(Resource* resource = nullptr);

void
Stop();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shutdown ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/engine.cpp Show resolved Hide resolved
src/resource_owner_wrapper.h Outdated Show resolved Hide resolved
jiaweizone
jiaweizone reviewed Dec 10, 2024
View reviewed changes
src/safe_allocator.h
@@ -68,6 +69,8 @@ class SafeAllocator : public Allocator {
private:
Allocator* const raw_allocator_ = nullptr;
std::shared_ptr<Allocator> owned_allocator_ = nullptr;

bool owner_{false};
Copy link
Collaborator

Choose a reason for hiding this comment

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

need deallocator to manage raw_allocator ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ditto

@LHT129
Copy link
Collaborator Author

LHT129 commented Dec 10, 2024

@LHT129 please add an example of this change, and also need create an issue about this feature

issue is necessary, but I will introduce several pull requests for this feature, this one is the first, the following is the example, test and remove the origin factory method

@LHT129 LHT129 force-pushed the factory branch 2 times, most recently from 9057005 to 4c33d66 Compare December 10, 2024 16:24
jiaweizone
jiaweizone approved these changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

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

LGTM

@LHT129 LHT129 mentioned this pull request Dec 10, 2024
Closed
@LHT129
new method to manage resource in engine
fa306a1 
Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
wxyucs
wxyucs approved these changes Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

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

lgtm

@LHT129 LHT129 merged commit 59587da into antgroup:main Dec 11, 2024
2 checks passed
@LHT129 LHT129 deleted the factory branch December 11, 2024 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxyucs wxyucs wxyucs approved these changes

@jiaweizone jiaweizone jiaweizone approved these changes

@inabao inabao Awaiting requested review from inabao inabao is a code owner

@ShawnShawnYou ShawnShawnYou Awaiting requested review from ShawnShawnYou ShawnShawnYou is a code owner

Assignees

@LHT129 LHT129

Labels
kind/feature New feature or request size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LHT129 @jiaweizone @wxyucs