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

Feat/refactor flags #6475

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Scoppio
Copy link
Collaborator

@Scoppio Scoppio commented Jan 31, 2025

What this PR does?

Changes every single BigInteger in MegaMek for an Enum, uses a specialized BitSet to substitute the BigInteger and emulates its API to make it possible to substitute with minimal changes in code all around the application.

This change reduces the amount of time it takes to go through the function EquipmentType::hasFlag which today is quite slow and is queried extremelly frequently.

How to use this PR?

This PR is standalone but probably MML and MHQ need to be updated, this is literally to get an "ok" on the change.

Improvements

I have tested using EnumSet for the flags, and a mix of Enum + BitSet, where the enum is a flag position in the bitSet and the bitset holds the flags itself. This was the result when running a Megamek game with 5 Princesses, no human players, for around 3 minutes. The same scenario was run the 3 times.

Flag Type Time Run Total Time Percent Normalized improvement
BigInteger 31,346ms 210,000ms 14.92% -
EnumSet 2,213ms 180,000ms 1.23% 12x
Enum+BitSet 72ms 180,000ms 0.04% 361x

The BitInteger, the original implementation, spent 31 seconds of the total 210 seconds of the test duration. With EnumSet the time spent querying hasFlag reduced to 2 secons, and with the Enum + BitSet the time was further reduced to 72ms.
With a few extra repetitions the result varied around 300ms and 50ms, depending on how many units were alive after the first couple of minutes.

@Scoppio Scoppio self-assigned this Jan 31, 2025
@Scoppio Scoppio requested a review from rjhancock January 31, 2025 23:11
@Scoppio Scoppio added the For New Dev Cycle This PR should be merged at the beginning of a dev cycle label Jan 31, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.65%. Comparing base (313ad5c) to head (77450ac).
Report is 9 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6475      +/-   ##
============================================
+ Coverage     28.56%   28.65%   +0.09%     
- Complexity    14373    14392      +19     
============================================
  Files          2798     2803       +5     
  Lines        275101   275456     +355     
  Branches      48669    48669              
============================================
+ Hits          78576    78940     +364     
+ Misses       191890   191881       -9     
  Partials       4635     4635              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

megamek/src/megamek/common/AmmoTypeFlag.java Outdated Show resolved Hide resolved
megamek/src/megamek/common/AmmoTypeFlag.java Outdated Show resolved Hide resolved
megamek/src/megamek/common/EquipmentBitSet.java Outdated Show resolved Hide resolved
megamek/src/megamek/common/MiscTypeFlag.java Outdated Show resolved Hide resolved
megamek/src/megamek/common/WeaponTypeFlag.java Outdated Show resolved Hide resolved
megamek/src/megamek/common/verifier/TestSmallCraft.java Outdated Show resolved Hide resolved
@Scoppio Scoppio requested a review from rjhancock February 1, 2025 02:58
@Scoppio Scoppio force-pushed the feat/refactor-flags branch from 38b7e6d to b122841 Compare February 1, 2025 02:59
@Scoppio
Copy link
Collaborator Author

Scoppio commented Feb 1, 2025

AmmoTypeFlag and WeaponTypeFlags reordered by family of weapon and alphabetized in each family.
Using ordinal instead of the original number values as the values are not persisted anywhere (will do a few more validations before we merge this one, and also may need changes for MML and MHQ, so this is not to be merged until both have their own branches.

Comment on lines +32 to +48

// Limit which type of unit can install the equipment
F_BA_EQUIPMENT,
F_MEK_EQUIPMENT,
F_TANK_EQUIPMENT,
F_FIGHTER_EQUIPMENT,
F_SUPPORT_TANK_EQUIPMENT,
F_PROTOMEK_EQUIPMENT,

F_JUMP_JET,
F_JUMP_BOOSTER,
F_MECHANICAL_JUMP_BOOSTER,

F_CASE,
F_CASEII,
F_CASEP,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Request: Add comments for each section of values for new people to know what kind of enums go into each section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was expecting for this comment XD

I'm not finished with it yet, too many equipment to figure out what each one of them is or means, so I am going slowly.

Copy link
Collaborator

@rjhancock rjhancock left a comment

Choose a reason for hiding this comment

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

Just the one requested change about the comments for the enum groupings.

@Scoppio Scoppio force-pushed the feat/refactor-flags branch from 6a22cd3 to 77450ac Compare February 1, 2025 04:02
@rjhancock
Copy link
Collaborator

Once done with the comments for the types within the enums, this should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For New Dev Cycle This PR should be merged at the beginning of a dev cycle Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants