Skip to content

Local allocator#1386

Merged
lockshaw merged 7 commits intoflexflow:repo-refactorfrom
reyna-abhyankar:local-allocator
May 28, 2024
Merged

Local allocator#1386
lockshaw merged 7 commits intoflexflow:repo-refactorfrom
reyna-abhyankar:local-allocator

Conversation

@reyna-abhyankar
Copy link
Collaborator

@reyna-abhyankar reyna-abhyankar commented May 14, 2024

Description of changes:

Local and tracked allocator

Related Issues:

Linked Issues:

  • Issue #

Issues closed by this PR:

  • Closes #

This change is Reviewable

@reyna-abhyankar reyna-abhyankar requested a review from lockshaw May 14, 2024 16:38
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyna-abhyankar)


lib/local-execution/include/tracked_allocator.h line 8 at r1 (raw file):

namespace FlexFlow {

struct TrackedAllocator : public Allocator {

TrackedAllocator should inherit from IAllocator. The way you have it implemented I think you're not actually overriding allocate, deallocate, etc. because they're not virtual, and making them virtual creates more problems than it solves. If you inherit from IAllocator then you can have an Allocator that holds a pointer to a TrackedAllocator which has a member which is an Allocator which holds a pointer to whatever the underlying allocator type is, which should behave correctly I think

Copy link
Collaborator Author

@reyna-abhyankar reyna-abhyankar left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @lockshaw)


lib/local-execution/include/tracked_allocator.h line 8 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

TrackedAllocator should inherit from IAllocator. The way you have it implemented I think you're not actually overriding allocate, deallocate, etc. because they're not virtual, and making them virtual creates more problems than it solves. If you inherit from IAllocator then you can have an Allocator that holds a pointer to a TrackedAllocator which has a member which is an Allocator which holds a pointer to whatever the underlying allocator type is, which should behave correctly I think

Done.

@reyna-abhyankar reyna-abhyankar added kernels Kernels library runtime Runtime library and removed kernels Kernels library labels May 23, 2024
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyna-abhyankar)


lib/local-execution/include/tracked_allocator.h line 8 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Done.

TrackedAllocator should wrap another allocator, rather than hardcoding in its own allocation logic, i.e., TrackedAllocator should have a field of type Allocator

Copy link
Collaborator Author

@reyna-abhyankar reyna-abhyankar left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @lockshaw)


lib/local-execution/include/tracked_allocator.h line 8 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

TrackedAllocator should wrap another allocator, rather than hardcoding in its own allocation logic, i.e., TrackedAllocator should have a field of type Allocator

Done.

Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @reyna-abhyankar)


lib/local-execution/include/tracked_allocator.h line 25 at r3 (raw file):

CHECK_RC_COPY_VIRTUAL_COMPLIANT(TrackedAllocator);

Allocator get_tracked_memory_allocator(Allocator base_allocator);

Suggestion:

Allocator get_tracked_memory_allocator(Allocator const &base_allocator);

Copy link
Collaborator Author

@reyna-abhyankar reyna-abhyankar left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @lockshaw)


lib/local-execution/include/tracked_allocator.h line 25 at r3 (raw file):

CHECK_RC_COPY_VIRTUAL_COMPLIANT(TrackedAllocator);

Allocator get_tracked_memory_allocator(Allocator base_allocator);

Done.

@lockshaw lockshaw enabled auto-merge (squash) May 28, 2024 05:12
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @reyna-abhyankar)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

runtime Runtime library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants