Skip to content

Conversation

mhilton
Copy link
Contributor

@mhilton mhilton commented Jun 4, 2024

Undo the main change introduced in #5377 so that by default strings returned from array.Strings use the internal memory of the stored data. This means that it is more likely that there are some cases of use-after-free errors in the flux executor, the trade-off being that there is less likely to be a proliferation of memory allocation due to string copying. Overall use-after-free errors will not cause incorrect results with regard to strings as they are read only and the Go runtime will only free the memory once the garbage collector has determined it is unused.

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues
  • 🏃 Test cases are included to exercise the new code
  • 🧪 If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/
  • 📖 If language features are changing, ensure docs/Spec.md has been updated

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

mhilton added 2 commits June 3, 2024 16:57
Undo the changes introduced in #5377. This is a trade-off where it
is considerd better to have as much memory as possible allocated
by the allocator, with the risk that it will be freed when still
in use. Than to have significant amounts of memory allocated outside
of the allocator.

It should be noted that because of Go's garbage collector it is
perfectly safe for memory to be "freed" when still in use. It just
mean that any accounting done by the allocator will be inaccurate.
Remove the recently added StringRef type as it is now unnecessary.
@mhilton mhilton requested a review from a team as a code owner June 4, 2024 07:24
@mhilton mhilton merged commit 32f7947 into master Jun 5, 2024
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.

3 participants