Skip to content

NFC: repack decl base #13284

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

Merged
merged 4 commits into from
Dec 6, 2017
Merged

Conversation

davezarzycki
Copy link
Contributor

Take advantage of unused bits in the Decl base class to repack subclass bits to save memory.

@davezarzycki
Copy link
Contributor Author

@swift-ci please test and merge

@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test and merge

@davezarzycki
Copy link
Contributor Author

@swift-ci please test and merge

1 similar comment
@davezarzycki
Copy link
Contributor Author

@swift-ci please test and merge

@swift-ci swift-ci merged commit c9c7d39 into swiftlang:master Dec 6, 2017
@davezarzycki davezarzycki deleted the nfc_repack_decl_base branch December 6, 2017 04:24
@jrose-apple
Copy link
Contributor

Does this actually save memory? Decl subclasses that don't need more than 32 bits are now paying for it.

@davezarzycki
Copy link
Contributor Author

davezarzycki commented Dec 11, 2017

For various reasons, yes.

  1. If one reviews the SCM history, there was a comment that the Decl base had 4 bytes of wasted padding after the bitfield.
  2. If we set the comment aside, natural alignment rules and the frequent use of pointers in data structures tends to make the vast majority of types be 64-bit aligned/padded on 64-bit architectures.
  3. Finally, computer vendors haven't sold 32-bit only workstations in many years. Therefore trying to optimize for 32-bit environments is fairly pointless.
  4. I've been using -Wpadded to verify memory savings.

@jrose-apple
Copy link
Contributor

Ah, if we didn't have anything else that could fit in those 32 bits, then this certainly makes sense. Okay, thanks, Dave!

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