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

Remove Primitive Optimization for Boolean Collections #2390

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

bmarcaur
Copy link
Member

@bmarcaur bmarcaur commented Oct 22, 2024

Before this PR

list<boolean> was optimized in this move to eclipse-collections, but its usage is almost non-existent (all instances appear to be some copy-paste from Excavator?). IMO, it's usage, or really this optimization, signals to devs that using a wire format of list<boolean> is a good idea, when a bit mask would be a much better abstraction for such a structure when it comes to our serialization.

On the other hand, it feels like an odd omission given the shape of the feature. Net net, I think removing it lowers our ownership overhead of this. boolean is a weird primitive type and when tackling changes for the 4 primitive types boolean is always a little unique. For example:

After this PR

Treated like any old boxed type, in the long term we should consider a bit mask type for an efficient representation of the usage pattern.

Possible downsides?

This may regress the peak memory usage on the small use cases, but also let's consider leveraging more expressive external types to experiment.

@bmarcaur bmarcaur marked this pull request as ready for review October 23, 2024 15:28
@bmarcaur bmarcaur force-pushed the bmarcaurele/remove-native-boolean-handling branch from 4bb64cb to 8c258cb Compare November 4, 2024 16:57
@bmarcaur bmarcaur requested a review from carterkozak November 4, 2024 20:02
@bmarcaur bmarcaur force-pushed the bmarcaurele/remove-native-boolean-handling branch from 8c258cb to b41a46d Compare November 5, 2024 17:57
Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@bulldozer-bot bulldozer-bot bot merged commit e7a024c into develop Nov 5, 2024
5 checks passed
@bulldozer-bot bulldozer-bot bot deleted the bmarcaurele/remove-native-boolean-handling branch November 5, 2024 18:51
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