Add new API function pcre2_set_optimization() for controlling enabled optimizations

[alexdowad]

It is anticipated that over time, more and more optimizations will be added to PCRE2, and we want to be able to switch optimizations off/on, both for testing purposes and to be able to work around bugs in a released library version.

The number of free bits left in the compile options word is very small. Hence, we will start putting all optimization enable/disable flags in a separate word. To switch these off/on, the new API function pcre2_set_optimization() will be used.

The values which can be passed to pcre2_set_optimization() are different from the internal flag bit values. The values accepted by pcre2_set_optimization() are contiguous integers, so there is no danger of ever running out of them. This means in the future, the internal representation can be changed at any time without breaking backwards compatibility. Further, the 'directives' passed to pcre2_set_optimization() are not restricted to control a single, specific optimization. As an example, passing PCRE2_FULL_OPTIMIZATION will turn on all optimizations supported by whatever version of PCRE2 the client program happens to be linked with.

[alexdowad]

There are obviously a couple of things missing here (notably, documentation), but I am pushing it to allow for review and comments.

[alexdowad]

One thing I would like to ask: Are there any other existing option bits which are used to turn optimizations on/off? Is it just the 3 which I have already identified?

[alexdowad]

Philip, Carlo, Zoltan: As usual, the code review on this project is great! Thanks very much... I will go through the comments one by one now.

[alexdowad]

Pushing an update which addresses many of the points raised by reviewers. I would love to ask once more: Do we have any other existing toggles which turn optimizations on/off? Is it just the 3 which I have included already?

[zherczeg]

Conversion is perfect, since it happens internally. During compile, the optimizations flags, which are already present as compile flags are "or'ed" to compile flags, so whichever is set, it activates the optimization (they don't need to have the same index). This should be easy since only a few flags are affected. For these optimizations the already present infrastructure will handle them (the optimization flags can be overwritten by compile flags). For newer optimizations, the optimization flagset is directly used.

[alexdowad]

Just a couple of thoughts here...

Right now, the default optimization behavior of PCRE2 is "enable all optimizations by default, disable selectively using option bits". For various reasons, it might become desirable to deviate from this behavior in the future.

Reason 1: There is more than one metric which can be optimized for. It's possible to optimize for small memory footprint at the cost of CPU time, or vice versa. Also, it's possible to optimize for low average CPU time at the cost of worst-case CPU time, or vice versa.

Reason 2: There may be certain "dangerous" optimizations, which improve performance at the cost of disabling certain features, or causing observable behavior changes. (For example: While DFA-based matching is desirable for consistent worst-case performance, the documentation for pcre2_dfa_match states that some features may behave slightly differently as compared to pcre2_match.)

For both of these reasons, I don't think we can assume that the default behavior will always be to enable all optimizations. Sooner or later, some may need to be opt-in rather than opt-out.

For this reason, I am thinking of adding a directive called PCRE2_OPTIMIZATION_DEFAULT which will restore the "default" settings, whatever those are. For now, PCRE2_OPTIMIZATION_DEFAULT would effectively be the same as PCRE2_OPTIMIZATION_FULL, but that might not be true forever.

Any comments?

[alexdowad]

OK, latest contributions from Carlo and Zoltan have been incorporated.

I hope to add tests and documentation tomorrow.

[PhilipHazel]

Alex wrote:

I wonder if it might not be a good idea to have some unit tests which are written in C, rather than as pcre2test scripts?? Just a thought...

I have no objection - we do already have pcre2_posix_test.c and pcre2_jit_test.c, but remember that any such program has to work with every possible combination of 8, 16, or 32 bit libraries.

[carenas]

For this reason, I am thinking of adding a directive called PCRE2_OPTIMIZATION_DEFAULT which will restore the "default" settings, whatever those are. For now, PCRE2_OPTIMIZATION_DEFAULT would effectively be the same as PCRE2_OPTIMIZATION_FULL, but that might not be true forever.

I don't see a use for this and even believe it is likely to cause confusion. The default is currently all on and when that changes you will still use PCRE2_OPTIMIZATION_FULL to override that default if you so desire so.

This API call (just like the other pcre2_set_* calls) are meant to be called only in a context that hasn't been used to compile the expression you want to affect and that will be followed by a pcre2_compile() call that uses that context. For the optimization flags, that compilation might disable some of them (using VERBS in the expression), but that context is done. Having that option (specially as some optimizations have effect at match time) might seem to imply that you can use it to affect the compiled expression optimizations further IMHO.

On second thought, if you are concerned about reusing the context WHEN the default is no longer full, it might be better to wait until then to add this IMHO.

[alexdowad]

OK, well, we have tests now.

Next is to add documentation.

[carenas]

OK, well, we have tests now.

The code is now using the new field, so why is pcre2test still setting the bits in the pcre2_compile() option parameter instead of calling the new API? for the original modifiers?

Sure; we can only do the API call if we have a context, so to test the "legacy" codepath need to probably piggyback on the null_context modifier, and we need a way to test turning off all optimizations so there is a need for the optimization_none modifier, but something more generic like optimization=0 might work better long term IMHO.

Still think that an API with an extra BOOL parameter to turn on/off the optimization makes more sense as well.

[alexdowad]

The code is now using the new field, so why is pcre2test still setting the bits in the pcre2_compile() option parameter instead of calling the new API?

PCRE2 needs to both support the new pcre2_set_optimization API, as well as the legacy option bits, so pcre2test now has modifiers which allow us to test both of those.

[zherczeg]

We should also test combinations, when a compile and optimization flag contradict to each other (compile flag should be stronger).

Since we might need to add lots of test for different optimizations in the future, we could even start a new input/output file for them.

[carenas]

now has modifiers which allow us to test both of those.

I wasn't arguing about having more modifiers, although was explaining that we could do with almost none by abusing what we currently have, but that the additional modifiers are only used sparingly and therefore the new code is barely exercised. But changing what the current modifiers do testing will be more comprehensive, and of course adding tests that go through the "legacy" path was also expected.

[alexdowad]

We should also test combinations, when a compile and optimization flag contradict to each other (compile flag should be stronger).

Ah, good point. Let me add that right now.

[alexdowad]

But changing what the current modifiers do testing will be more comprehensive, and of course adding tests that go through the "legacy" path was also expected.

Ah, true. That is a good point.

Hmm. I do think that for PCRE2 developers, it makes things a bit easier if the pcre2test modifiers have the same name as the corresponding option, and the names of the pcre2_set_optimization() directives are currently like PCRE2_AUTO_POSSESS_OFF...

Hmm. 🤔

[alexdowad]

Ah, good point. Let me add that right now.

Done, you'll see it on the next push.

[alexdowad]

But changing what the current modifiers do testing will be more comprehensive

Just thinking about this...

The tests do demonstrate that pcre2_set_optimization turns the optimization flags on and off as expected. Since the code for pcre2_set_optimization is almost trivially simple, is that not enough?

I think what needs more thorough testing is that the actual optimizations behave as expected and do not break anything. But we already have existing tests for that. Right now we are just testing a different way of switching them on and off.

[alexdowad]

OK, well, I have drafted the needed documentation as well. Code like if (value & FLAG) has been edited to if ((value & FLAG) != 0). As per suggestion from Carlo, PCRE2_OPTIMIZATION_DEFAULT has been removed. Carlo's suggestion to add another API function for turning all optimizations on or off is appreciated, but I think it is better to just use pcre2_set_optimize.

Can anyone see anything else which has been missed??

[carenas]

Carlo's suggestion to add another API function for turning all optimizations on or off is appreciated, but I think it is better to just use pcre2_set_optimize

I never suggested that, but instead to change the current API so it has instead an interface:

pcre2_set_optimize(pcre2_compile_context *, int optimization, int enable)

so with that you would disable all optimizations with pcre2_set_optimize(c, PCRE2_OPTIMIZE_FULL, 0) and only need 1 additional entry and modifier for each optimization to support.

[alexdowad]

I never suggested that, but instead to change the current API so it has instead an interface: ...

so with that you would disable all optimizations with pcre2_set_optimize(c, PCRE2_OPTIMIZE_FULL, 0) and only need 1 additional entry and modifier for each optimization to support.

Ah, this is interesting. Yes, I can see some advantages of using an interface like that.

On the other hand, I can also see benefits to the current design. Let's say that later on we want to add an optimization directive which says: "Optimize for small memory footprint". What would the negation of that be? "Optimize for large memory footprint"?

At the moment, all of the optimization options make sense when negated, but that might not always be the case.

[carenas]

Right now we are just testing a different way of switching them on and off.

No; all the code that looks at when to do the optimizations has been changed as well to only look at the new optimization_flag exclusively, and all the tests (except the new ones) use the "legacy" modifiers that ONLY set the bits on the no longer looked upon variables.

I understand that we want to test both, but it makes more sense to me that we will prioritize the new API (when possible, ex: unless there is no compile context) and try to maximize the chances to exercise the new code instead, after all pcre2test is a test program.

[alexdowad]

No; all the code that looks at when to do the optimizations has been changed as well to only look at the new optimization_flag exclusively, and all the tests (except the new ones) use the "legacy" modifiers that ONLY set the bits on the no longer looked upon variables.

How about this; for every one of the current optional optimizations which affects the regex bytecode, I will add tests which use the B modifier to demonstrate that the optimization happens/does not happen as expected when pcre2_set_optimize is used.

[carenas]

No; all the code that looks at when to do the optimizations has been changed as well to only look at the new optimization_flag exclusively, and all the tests (except the new ones) use the "legacy" modifiers that ONLY set the bits on the no longer looked upon variables.

How about this; for every one of the current optional optimizations which affects the regex bytecode, I will add tests which use the B modifier to demonstrate that the optimization happens/does not happen as expected when pcre2_set_optimize is used.

This should had been done already by the current tests at least implicitly (didn't check though), but the output of the B modifier doesn't protect the code from a future modification that might mistakenly look at the now defunct bits for those optimizations (assuming that all the proposed code does, which I think I verified in a previous revision with a hacked version).

[zherczeg]

I would not add an enable (or any operation opcode) to the api, since 2^32 is a huge space, and we can create optimization groups, which are handled in the same way internally. For a single optimization, enable/disable is ok, but for complex ones, we might want to SET, ADD, REMOVE optimization lists. ADD==ENABLE, REMOVE==DISABLE, but SET cannot be passed.

We can add more api functions, if we really need later, e.g. if an optimization has a configuration argument.

[alexdowad]

@PhilipHazel Just discovered something here...

It seems that auto dotstar anchoring is handled as a "start-up optimization", so START_OPTIMIZE_OFF effectively disables it. It's like START_OPTIMIZE_OFF "implies" DOTSTAR_ANCHOR_OFF.

Is that intentional? If so, I think the documentation should be more clear about it.

[alexdowad]

Aside from the issue I have just raised about auto dotstar anchoring, I have just added more tests confirming that the new function disables optimizations as expected.

Can anyone offer feedback on the documentation changes in this PR?

[PhilipHazel]

Re dotstar anchoring: It is more complicated than you have realized. Setting NO_START_OPTIMIZE does not stop the compiler marking the pattern as anchored if it starts with .* and DOTALL is set:

PCRE2 version 10.45-DEV 2024-06-09 (8-bit)
/.*abc/info,dotall,no_start_optimize
Capture group count = 0
Compile options: dotall no_start_optimize
Overall options: anchored dotall no_start_optimize

What NO_START_OPTIMIZE turns off at match time is a particular check of the first code unit when the pattern is anchored.

[PhilipHazel]

Documentation changes. I have not scrutinized these in detail, but what I noticed is that you are changing the overall style of the documentation. All the previously existing functions are described in detail in pcre2api.3, with the man pages for the individual functions just summarizing the API for each function. You have put the full description into the latter, with just a cross-reference in pcre2api.

One can argue this either way - the current situation developed from the early releases where there were far fewer functions - but I feel the there should be some consistency. I think on balance I prefer the current style because one can scroll around pcre2api and get information about many functions rather than having to look up multiple pages (either as man pages or HTML). The downside is that pcre2api is now rather lengthy.

[alexdowad]

Re dotstar anchoring: It is more complicated than you have realized...

Hmm. OK, thanks for the explanation. I hope to get to know these details better with time.

[alexdowad]

Documentation changes. I have not scrutinized these in detail, but what I noticed is that you are changing the overall style of the documentation...

Thanks for the feedback. :+1: Please check the update which I have just pushed.

[PhilipHazel]

On a fairly quick read (I know we want to get on with this) that looks good. Minor style nitpick is that you refer to pcre2_match rather than pcre2_match(). Are we now ready to merge this PR?

[alexdowad]

On a fairly quick read (I know we want to get on with this) that looks good. Minor style nitpick is that you refer to pcre2_match rather than pcre2_match().

Thanks for noting that. Fixed.

[alexdowad]

I know we want to get on with this

Yes, we do want to get on with this, but to the extent possible, we also want to get the API "right" the first time rather than making breaking changes later. So by all means, let's invite more comments from reviewers especially if they concern the design of the API, as visible to client programmers.

Non-breaking improvements to the implementation can be done any time.

[alexdowad]

Converted pcre2_set_optimize to use switch as suggested by Carlo.

[alexdowad]

Just pushed another update.

[alexdowad]

Pushed another update.

[alexdowad]

Can others share any further comments on what error code to return from pcre2_set_optimize() if an invalid directive is passed? Right now it's PCRE2_ERROR_BADOPTION. Should I switch to another one, or create a new error code specifically for this case?

[zherczeg]

https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_match.c#L6764

PCRE2_ERROR_BADOPTION means the option is unknown, although the name is a bit strong. PCRE2_ERROR_NULL is returned if the pattern is not set. That means the current return values are ok for me.

For the function body, the large switch does not look nice for me. For generic options, such as FULL, the switch is perfect, but for option bits, I liked the old code (which could be placed in the default case, and returns with BADOPTION if the bit is not known).

As for LITERAL, it is ok if the compile limits the flags (we should not change this), but for compile context, which is a generic configuration store, there should be no limitations. The unrelated bits can be simply ignored. This is true for other members: if malloc is not needed by a function, it simply does not use it.

[alexdowad]

Just updating code to match Zoltan's preference on the use of switch...

[alexdowad]

Pushed another update.

[carenas]

Can anyone see anything else which has been missed??

A minor issue and easy to miss is that the generated html documentation is not linked correctly; had prepared alexdowad/pcre2#7 where the first commit show the missing bits that could be cherry-picked over to fix it.

That branch is a little old, and so the continuous rebasing of this PR makes it an odd target for a simple merge/rebase but hope you find also the other commits useful otherwise.

[alexdowad]

Incorporated the addition to index.html (sorry that I missed that point) and some other helpful additions from Carlo. Pushed another update.

[PhilipHazel]

Is this ready to go now?

[zherczeg]

It is good for me. If something comes up, we can fix it later. Before the next release, we can even change the api if needed, but I think we discussed everything.

[carenas]

I think we discussed everything

I think that considering there are almost 150 comments, we probably discussed too much, but also run the risk of having overlooked, misunderstood or not resolved some of the points made as well.

Merging it now, will hopefully help having a less volatile base to work on though, and hopefully will lead to less clutter in the communication about them.

[alexdowad]

Is this ready to go now?

Yes, it's ready.