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.

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

Change prefix stack to List[String] #1617

Merged
merged 2 commits into from
Oct 19, 2020
Merged

Change prefix stack to List[String] #1617

merged 2 commits into from
Oct 19, 2020

Conversation

jackkoenig
Copy link
Contributor

This allows for sharing of the prefix stacks between Data, and removes a
boxing per prefix stack element that is no longer necessary.

My intuition is that this should reduce memory by quite a bit and thus improve performance, but I'd like to try it out on complex designs before we merge.

This should improve memory and performance for a few reasons:

  1. construction_prefix for each Data is now free (it formerly copied from ArrayBuffer to List)
  2. Structural sharing of common prefix stacks and intermediate prefix stacks
  3. No more Either in prefix stack, removes an object allocation per stack element

While we do have to copy the prefix stack when we reverse for naming, this is a copy on naming of Data rather than on construction. In theory, most Data should not be named, but due to the fact that every HasId adds itself to the ids of its parent module, every Data does currently get named. Once that latter issue is resolved, this will result in far fewer copies of the prefix stack. It is still better with this change because the copies can be GCed right away rather than sticking around being referred to by each Data. We can also consider memoizing the prefix stack reversals.

Contributor Checklist

  • [NA] 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

  • performance improvement

API Impact

No impact

Backend Code Generation Impact

No impact

Desired Merge Strategy

  • Squash

Release Notes

Reduce memory overhead from better naming improvements in v3.4.0

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

This allows for sharing of the prefix stacks between Data, and removes a
boxing per prefix stack element that is no longer necessary.
@jackkoenig jackkoenig added this to the 3.4.x milestone Oct 13, 2020
@jackkoenig jackkoenig requested a review from azidar October 13, 2020 02:21
Copy link
Contributor

@azidar azidar left a comment

Choose a reason for hiding this comment

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

As is LGTM. I don't think you need a test because you aren't changing any behavior.

@jackkoenig jackkoenig marked this pull request as ready for review October 19, 2020 18:17
@jackkoenig jackkoenig requested a review from a team as a code owner October 19, 2020 18:17
@jackkoenig jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Oct 19, 2020
@mergify mergify bot merged commit fef0b68 into master Oct 19, 2020
mergify bot pushed a commit that referenced this pull request Oct 19, 2020
This allows for sharing of the prefix stacks between Data, and removes a
boxing per prefix stack element that is no longer necessary.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit fef0b68)
@mergify mergify bot added the Backported This PR has been backported label Oct 19, 2020
mergify bot added a commit that referenced this pull request Oct 20, 2020
* Change prefix stack to List[String] (#1617)

This allows for sharing of the prefix stacks between Data, and removes a
boxing per prefix stack element that is no longer necessary.

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

* Add binary compatibility waivers

Co-authored-by: Jack Koenig <[email protected]>
@jackkoenig jackkoenig deleted the prefix-stack-list branch October 20, 2020 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

2 participants