Skip to content

Conversation

@jsternberg
Copy link
Contributor

@jsternberg jsternberg commented Mar 8, 2022

This converts the allocator to an interface so we can wrap it in
different locations. It expands on the arrow allocator to add a direct
access to the account method of the underlying resource allocator.

The original plan was to separate these two so the allocator would not
have an Account() method. This proved to be too difficult as the
Account() method is used in some very deep places in the code and
changing all of those locations was just not practical.

Fixes #4472.

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@jsternberg jsternberg requested a review from a team as a code owner March 8, 2022 16:10
@jsternberg jsternberg requested review from onelson and removed request for a team March 8, 2022 16:10
@jsternberg jsternberg force-pushed the refactor/arrow-memory-allocator branch 2 times, most recently from c598315 to 84f2940 Compare March 8, 2022 17:24
Copy link
Contributor

@onelson onelson left a comment

Choose a reason for hiding this comment

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

The diff looks good. Questions for my own edification.

I was a little unsure about when we should be favoring GoAllocator versus ResourceAllocator or even relying on the Allocator interface. Maybe there could be a short explainer in the module-level docstring for memory.

In the diff, you change to use ResourceAllocator some of the time, but not all of the time. Some text about the rationale might help.

// Allocator defines how memory is allocated and released within
// the flux engine.
type Allocator interface {
memory.Allocator
Copy link
Contributor

Choose a reason for hiding this comment

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

Go lang question: seems like this is saying "this interface should match the fields on this other struct" with this line. Is this sort of like serde's flatten attribute?

Also, I was a little confused since this is the memory package, so it seemed ambiguous as to what was going on here. My guess is memory.Allocator here is referring to arrow's memory, but it took me a beat to realize this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, memory references arrow's memory package. What this is saying is "This interface needs to implement this other interface and can be used in places that are expecting that interface." It's not really like flatten but it's a builtin part of the language.

You can read about it in the language spec: https://go.dev/ref/spec#Interface_types.

@jsternberg
Copy link
Contributor Author

GoAllocator is the name of the same type in arrow that implements a version of the allocator that just uses make(). Since we're adding to the interface, it's just creating a version of that that works with our interface.

@jsternberg jsternberg force-pushed the refactor/arrow-memory-allocator branch from 84f2940 to 1e13eed Compare March 8, 2022 21:15
@jsternberg jsternberg force-pushed the refactor/arrow-memory-allocator branch 2 times, most recently from 3a47f85 to ee61ce0 Compare March 21, 2022 17:28
This converts the allocator to an interface so we can wrap it in
different locations. It expands on the arrow allocator to add a direct
access to the account method of the underlying resource allocator.

The original plan was to separate these two so the allocator would not
have an `Account()` method. This proved to be too difficult as the
`Account()` method is used in some very deep places in the code and
changing all of those locations was just not practical.
@jsternberg jsternberg force-pushed the refactor/arrow-memory-allocator branch from ee61ce0 to f42eba2 Compare March 21, 2022 17:29
@jsternberg jsternberg merged commit 89d78e0 into master Mar 21, 2022
@jsternberg jsternberg deleted the refactor/arrow-memory-allocator branch March 21, 2022 19:22
jsternberg added a commit to influxdata/influxdb that referenced this pull request Mar 21, 2022
jsternberg added a commit to influxdata/influxdb that referenced this pull request Mar 22, 2022
jsternberg added a commit to influxdata/influxdb that referenced this pull request Mar 22, 2022
jsternberg added a commit to influxdata/influxdb that referenced this pull request Mar 22, 2022
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.

Refactor memory allocator to an interface and remove direct references to the current allocator

3 participants