Optimize out unnecessary eclass bitmaps #596

zherczeg · 2024-12-05T10:54:20Z

Large patch, contains code moves and reworks.

zherczeg · 2024-12-05T11:13:31Z

src/pcre2_compile_class.c

@@ -2276,12 +2288,11 @@ return TRUE;
 /* This function consumes unary prefix operators. */

 static BOOL
-compile_class_unary(uint32_t options, uint32_t xoptions, BOOL negated,
+compile_class_unary(eclass_context *context, BOOL negated,


This could be merged with compile_class_juxtaposition.

It's clear as it is. I don't mind either way.

Maybe later. Not in this patch.

NWilson

Nice! That much easier than I'd feared it would be. Thank you.

src/pcre2_xclass.c

src/pcre2_compile_class.c

NWilson · 2024-12-05T12:52:47Z

src/pcre2_compile_class.c

@@ -2143,6 +2173,7 @@ switch (meta)
    if (lengthptr != NULL)
      *lengthptr += code - (code_start + 1);
    code = code_start + 1;
+    *context->eclass_start |= CLASS_ECLASS_HAS_BITMAP;


Technically you could add a check, for whether any bits in the bitmap are actually set. If the user writes [^\x00-\xFF] then it will generate an OP_NCLASS with no bits set, and the eclass won't need a bitmap.

If you wanted to add the check it would be "for ECL_NONE see if bitmap is all zero; for ECL_ANY see if the bitmap is all set"

I am not sure I follow. The OP_ALLANY is handled above, and the OP_CLASS with all zero bits can be optimized. Nothing else can be optimized. Am I right?

The condition to leave out the map is: if you emit ECL_ANY, does the pop_info have an all-ones bitmap? And if you emit an ECL_NONE, does the pop_info have an all-zeros bitmap?

One of the OP_NCLASS branches could hit that condition, so doesn't always need a bitmap.

If ECL_ANY is set, and all bits are set, the compile_class_not_nested already converts the XCLASS to OP_ALLANY, and handled earlier. Hence the ECL_NONE and all bits are cleared are the remaining case. Then my question was correct.

Great, we agree! You've special-cased the OP_NCLASS case, which (out of OP_CLASS & OP_NCLASS) is the only one where it's possible to not set need_bitmap.

NWilson · 2024-12-05T12:57:16Z

src/pcre2_compile_class.c


    PCRE2_ASSERT(*code_start == OP_XCLASS);
    *code_start = pop_info->op_single_type = ECL_XCLASS;

    PCRE2_ASSERT(code - code_start >= 1 + LINK_SIZE + 1);
    flags = code_start[1 + LINK_SIZE];

-    if ((flags & XCL_MAP) != 0)
+    if ((flags & XCL_MAP) != 0 ||


I find it confusing that you write out the XCL_MAP flag, but then don't write out the bitmap (leaving it in cb->classbits instead), and then check & clear the flag immediately afterwards.

Is there not a cleaner way to do it? (I admit - it was ugly, the way I was rewriting the bytecode before.) If not, then it needs a comment a least.

I have replaced it with a BOOL* pointer.

Thanks, that makes me happy!

src/pcre2_compile_class.c

NWilson · 2024-12-05T13:01:03Z

src/pcre2_compile_class.c

@@ -2276,12 +2288,11 @@ return TRUE;
 /* This function consumes unary prefix operators. */

 static BOOL
-compile_class_unary(uint32_t options, uint32_t xoptions, BOOL negated,
+compile_class_unary(eclass_context *context, BOOL negated,


It's clear as it is. I don't mind either way.

NWilson · 2024-12-05T13:04:35Z

src/pcre2_compile_class.c

  negated = !negated;

 /* Because it's a non-empty class, there must be an operand at the start. */
-if (!compile_class_binary_loose(options, xoptions, negated, &ptr, &code,
-                                pop_info, errorcodeptr, cb, lengthptr))
+(*pptr)++;


This (*pptr++) is placed confusingly under the comment now. But this skip has nothing to do with the operand, it's the META_CLASS we're skipping.

NWilson · 2024-12-05T13:05:53Z

src/pcre2_compile_class.c

@@ -2292,15 +2303,13 @@ while (*ptr == META_ECLASS_NOT)
  negated = !negated;
  }

+*pptr = ptr;


Moving up the pptr assignment breaks the pattern with the other functions. Although, it looks like it behaves the same.

It is an optimization. There is no need to cache the pptr.

NWilson · 2024-12-05T13:08:24Z

src/pcre2_compile_class.c

+PCRE2_UCHAR *previous;
+BOOL allbitsone = TRUE;
+
+context.eclass_start = *pptr;


Storing this bit in the META code seems very dangerous. It carries the risk that different branches will be taken on the two passes through the code.

The flag you want should just be a BOOL on the eclass_context, so it's got a fresh state on each pass.

That was the whole point of the patch. Although I realized you inject the bitmap at the end, so caching this value is likely unnecessary.

src/pcre2_compile_class.c

zherczeg · 2024-12-05T14:35:59Z

I tried one of your suggestions. Original code:

  re> /[\pL&&[^\x00-\xFF]]/B,utf,alt_extended_class
------------------------------------------------------------------
        Bra
        [\p{L}]
        Ket
        End
------------------------------------------------------------------
data> a
 0: a

This should not happen. Interestingly, this patch fixes it. Not sure what is happening.

zherczeg · 2024-12-05T14:46:56Z

I have resolved the some conversions, but feel free to reopen them if they are not resolved. There is one comment I don't understand. Let me know if other changes are needed.

zherczeg · 2024-12-05T15:52:13Z

I hope I fixed everything. I noticed that it is not possible to tell the difference in the byte code dump that if an xclass has no bitset, or all bits in the bitset are zero. Maybe an (allzero) could be added after [] if a bitset is present, and everything is 0. I can do this in another patch.

NWilson · 2024-12-05T19:50:40Z

src/pcre2_compile_class.c

@@ -2125,6 +2155,7 @@ switch (meta)
    pop_info->length = 1;
    *code_start = pop_info->op_single_type = ECL_ANY;
    memset(pop_info->bits.classbits, 0xff, 32);
+    context->needs_bitmap = TRUE;


I believe that this OP_ALLANY → ECL_ANY branch does not need to set needs_bitmap.

NWilson · 2024-12-05T19:53:34Z

src/pcre2_compile_class.c

@@ -1752,6 +1774,8 @@ whole class was negated and whether there were negative specials such as \S
 (non-UCP) in the class. Then copy the 32-byte map into the code vector,
 negating it if necessary. */

+PCRE2_ASSERT(classbits == cb->classbits.classbits);


We could lose both of these assertions if you want. We only ever assign once to the classbits variable (it's const) so there's really not much to check.

There are many places throughout the function where we assume that classbits and classwords are the same.

Or make it a uint8_t *const classbits = ... instead.

Ok. I have added a comment about it as well.

NWilson · 2024-12-05T20:13:22Z

I tried one of your suggestions. Original code:
  re> /[\pL&&[^\x00-\xFF]]/B,utf,alt_extended_class
------------------------------------------------------------------
        Bra
        [\p{L}]
        Ket
        End
------------------------------------------------------------------
data> a
 0: a
This should not happen. Interestingly, this patch fixes it. Not sure what is happening.

Ouch! I have diagnosed the cause of this, in the code that's currently on master.

There are two cases where we emit a bitmap: there's the branch (previously in pcre2_compile.c) which emits an actual OP_ECLASS, and there's the branch that converts a single ECL_XCLASS into an actual OP_XCLASS. Both of these were using the same check: need_map = !allbitszero;

For OP_ECLASS, that (was) correct - no bitmap meant the same as all-bits-zero. But for OP_XCLASS it was not true. So there was an existing bug in my code you fixed.

The new logic checks context.needs_map in both branches, which is good - because OP_ECLASS and OP_XCLASS now have the same semantics for when a map is not provided.

Great! I'm glad I understand the bug, and can verify that you've properly fixed it, not just hidden it.

NWilson

I've left two additional small comments. Thank you for working on this! You can merge whenever you're ready (with or without those small suggestions).

zherczeg · 2024-12-06T04:03:37Z

I am happy that one less bug in the code. The test case was your suggestion, I just tried it.

zherczeg force-pushed the optimize_bitmap branch from cc180a5 to 1160e8d Compare December 5, 2024 11:07

zherczeg commented Dec 5, 2024

View reviewed changes

zherczeg force-pushed the optimize_bitmap branch from 1160e8d to e90ba81 Compare December 5, 2024 11:17

NWilson reviewed Dec 5, 2024

View reviewed changes

zherczeg force-pushed the optimize_bitmap branch from e90ba81 to ef5c288 Compare December 5, 2024 14:40

zherczeg force-pushed the optimize_bitmap branch 3 times, most recently from 700e029 to a9a8a4b Compare December 5, 2024 15:47

NWilson reviewed Dec 5, 2024

View reviewed changes

NWilson approved these changes Dec 5, 2024

View reviewed changes

Optimize out unnecessary eclass bitmaps

f1690d7

zherczeg force-pushed the optimize_bitmap branch from a9a8a4b to f1690d7 Compare December 6, 2024 04:00

zherczeg merged commit f95b881 into PCRE2Project:master Dec 6, 2024
21 checks passed

zherczeg deleted the optimize_bitmap branch December 6, 2024 04:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize out unnecessary eclass bitmaps #596

Optimize out unnecessary eclass bitmaps #596

zherczeg commented Dec 5, 2024

zherczeg Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

NWilson left a comment

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

NWilson Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

NWilson Dec 5, 2024

NWilson Dec 5, 2024

NWilson Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 5, 2024

zherczeg commented Dec 5, 2024

zherczeg commented Dec 5, 2024

zherczeg commented Dec 5, 2024

NWilson Dec 5, 2024

zherczeg Dec 6, 2024

NWilson Dec 5, 2024 •

edited

Loading

zherczeg Dec 6, 2024

NWilson commented Dec 5, 2024

NWilson left a comment

zherczeg commented Dec 6, 2024

Optimize out unnecessary eclass bitmaps #596

Optimize out unnecessary eclass bitmaps #596

Conversation

zherczeg commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NWilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg commented Dec 5, 2024

zherczeg commented Dec 5, 2024

zherczeg commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NWilson Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NWilson commented Dec 5, 2024

NWilson left a comment

Choose a reason for hiding this comment

zherczeg commented Dec 6, 2024

NWilson Dec 5, 2024 •

edited

Loading