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

feature.*: allow cop_feature to expand #22824

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

tonycoz
Copy link
Contributor

@tonycoz tonycoz commented Dec 5, 2024

The initial implementation of feature bits for faster access to feature flags used a single U32 member in COP to store the bits, but we're now up to 24 features, so rather than leaving this to the last minute I've re-worked regen/feature.pl to allow multiple U32 fields.

If you want to test this effectively you'll need to:

  • add some features to %feature in regen/feature.pl, I added:
      "add1" => "add1",
      "add2" => "add2",
      "add3" => "add3",
      "add4" => "add4",
      "add5" => "add5",
      "add6" => "add6",
      "add7" => "add7",
      "add8" => "add8",
      "add9" => "add9",
      "add10" => "add10",
      "add11" => "add11",
      "add12" => "add12",
      "add13" => "add13",
      "add14" => "add14",
      "add15" => "add15",
  • make regen
  • update COP_FEATURE_SIZE in cop.h

and test.

The low alphabet names here force some existing features into the second word so you're actually testing access and handling of that second word.


  • This set of changes does not require a perldelta entry.

The initial implementation of feature bits for faster access to
feature flags used a single U32 member in COP to store the bits, but
we're now up to 24 features, so rather than leaving this to the last
minute I've re-worked regen/feature.pl to allow multiple U32 fields.
@jkeenan jkeenan requested review from iabyn and ilmari December 13, 2024 16:00
@leonerd
Copy link
Contributor

leonerd commented Dec 16, 2024

I wonder if it would make feature.h a bit less huge, if instead of storing a U32 mask and index per feature, we just had one "number" per feature and compiled the mask tests into something like:

if(cop.feature_bits[(num) / 32] & (1 << ((num) % 32)) { ... }

Those div and mod operators would be all operating on compiletime constants so they'd decompose into basically the same values we have anyway, and I think it'll all get compiled into the same stuff; just we'd only be storing one number per feature, as we do now, rather than those two separate ones.

@tonycoz
Copy link
Contributor Author

tonycoz commented Dec 17, 2024

I wonder if it would make feature.h a bit less huge,...

I'm a bit indecisive here - I've looked at the pre-processed source often enough that I prefer simpler macros (though the stable has long since burned down here for the perl source), but a single source of truth for the bits are good too.

#define COP_FEATURE_SIZE 1

/* make this a struct so we can copy the feature bits with assignment */
struct cop_feature_t {

Choose a reason for hiding this comment

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

I'd go with:

typedef U32 cop_feature_block_t;
typedef cop_feature_block_t cop_feature_t [COP_FEATURE_SIZE];

for macros:

#define FEATURE_A 0
...
#define FEATURE_B 126
#define FEATURE_MAX_127


#define COP_FEATURE_SIZE ((COP_FEATURE_MAX / sizeof (cop_feature_block_t))

#define bitsizeof(X) (8 * sizeof (X))
#define IS_FEATURE_ENABLED(Flags, Feature) \
   Flags[Feature / bitsizeof (cop_feature_block_t)] & (1 << (Feature % bitsizeof (cop_feature_block_t))
....

For constants operations are evaluated at compile time.
Macros can adapt to best int size, for example:

typedef
#if COP_FEATURE_MAX > bitsize (U32) && U64_BITOP_IS_FASTER_THAN_U32
U64
#else
U32
#endif
cop_feature_block_t;

@tonycoz tonycoz merged commit 87e1d17 into Perl:blead Jan 9, 2025
34 checks passed
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.

4 participants