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

Micro-optimize Direction #4251

Merged
merged 2 commits into from
Jul 10, 2024
Merged

Conversation

jackkoenig
Copy link
Contributor

This almost entirely API compatible, but due to ActualDirection.Bidirectional being a case class, this change breaks things like the copy method. This is mostly a performance improvement

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 request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • API modification
  • Performance improvement

Desired Merge Strategy

  • Rebase (merge commit)

Release Notes

  • Specified and actual direction information are each now stored as single bytes rather than references.
  • This reduces the memory use of a typical bound UInt from 72 bytes shallow, 128 bytes retained to 64 bytes shallow, 120 bytes retained.
  • The change is mostly source compatible, but ActualDirection.Bidirectional, has changed slightly to memoize its two possibilities (Bidirectional.Default and Bidirectional.Flipped). There are deprecations for the typical APIs

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

This isn't visible in the public API, but saves 3 or 7 bytes (depending
on heap size) per Data.
This does have a minor impact on public API around
ActualDirection.Bidirectional, but it's source compatible.

This saves 3 or 7 bytes (depending on heap size) per Data with
non-bidirectional ActualDirection (all bound hardware ends up with an
ActualDirection).

This saves ~20 bytes per Bidirectional Data.

Move _directionVar to be next to _specifiedDirection

This may help with alignment and reducing memory footprint.
@jackkoenig jackkoenig force-pushed the jackkoenig/microoptimize-direction branch from 40d2fcc to a07b8d5 Compare July 10, 2024 00:58
@jackkoenig jackkoenig merged commit 9587312 into main Jul 10, 2024
14 checks passed
@jackkoenig jackkoenig deleted the jackkoenig/microoptimize-direction branch July 10, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants