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

Optimize memory use of naming prefixes #2471

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Optimize memory use of naming prefixes #2471

merged 3 commits into from
Apr 12, 2022

Conversation

jackkoenig
Copy link
Contributor

  • Use a single field instead of two in HasId (4-bytes per HasId)
  • Set the prefix to Nil after setting ref to free up memory

Draft PR for now because I'm having a bit of trouble measuring a memory improvement from this (perhaps I should write a synthetic benchmark), but analytically, it is easy to see how it should be beneficial.

This also has minor bug potential because the behavior of _computeName will change if someone calls it after forceName is called on a HasId. Auditing the code, that doesn't happen, but it might be worth having _computeName get the name from the _ref if it's available or perhaps error if called when it shouldn't be.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@jackkoenig
Optimize memory use of naming prefixes
cdf69ac 
* Use a single field instead of two in HasId (4-bytes per HasId)
* Set the prefix to Nil after setting ref to free up memory
@jackkoenig jackkoenig marked this pull request as ready for review April 5, 2022 19:48
@jackkoenig jackkoenig added this to the 3.5.x milestone Apr 5, 2022
seldridge
seldridge approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

lgtm

Why have two mutable vars when you can just use one and overwrite it? 😉

@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Apr 11, 2022
@mergify mergify bot merged commit 3aa179f into master Apr 12, 2022
@mergify mergify bot deleted the optimize-prefix-memory-use branch April 12, 2022 00:14
mergify bot pushed a commit that referenced this pull request Apr 12, 2022
@jackkoenig
Optimize memory use of naming prefixes (#2471)
fdbe81f 
* Use a single field instead of two in HasId (4-bytes per HasId)
* Set the prefix to Nil after setting ref to free up memory

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 3aa179f)
@mergify mergify bot mentioned this pull request Apr 12, 2022
Merged
@mergify mergify bot added the Backported This PR has been backported label Apr 12, 2022
mergify bot added a commit that referenced this pull request Apr 12, 2022
@mergify @jackkoenig
Optimize memory use of naming prefixes (#2471) (#2480)
efd4748 
* Use a single field instead of two in HasId (4-bytes per HasId)
* Set the prefix to Nil after setting ref to free up memory

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 3aa179f)

Co-authored-by: Jack Koenig <koenig@sifive.com>
@sequencer sequencer mentioned this pull request May 3, 2022
Open
14 tasks
jackkoenig pushed a commit that referenced this pull request Feb 28, 2023
@scala-steward @mergify
Update scalacheck-1-15 to 3.2.11.0 (#2471)
a606471 
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seldridge seldridge seldridge approved these changes

Assignees
No one assigned
Labels
Backported This PR has been backported Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Milestone
3.5.x
Development

Successfully merging this pull request may close these issues.
2 participants
@jackkoenig @seldridge