[fpc-devel] Const optimization is a serious bug (What nexy)

[Martin]

On 08/07/2011 11:05, Jonas Maebe wrote:
> On 08 Jul 2011, at 05:51, Chad Berchek wrote:
>> I'm more interested now in a solution. The solutions I've seen so far 
>> have potential, but in my opinion most of them seem like they are 
>> more complex, would be more overhead, and produce worse performance 
>> than just getting rid of the const "optimization".
>
> The main proposals I've seen were
> a) treat const string parameters the same as value parameters (i.e., 
> get rid of the current const behaviour)
> b) the same as a), combined with a very conservative heuristic to 
> apply the optimization in case there can be no side effects. I.e., in 
> case there is not a single function call and not a single indirect 
> write (var parameter, pointer, global variable, dynamic array, ...) to 
> any string or anything that might be somehow aliased to a string 
> passed as parameter to the current routine
> c) keep the current behaviour, but add functionality to the compiler 
> to help debug problems that can occur as a result of problems that can 
> occur as a result of this behaviour
>
Actually I think you can describe that as 3 independent proposals:

1) Add a compiler switch or directive to disable "const"behaviour for 
ref counted types (e.g. simply ignore "const" for 
string/dyn-array/interface params; Actually downgrade, still prevent 
assignment to local var)
2) Add automatic optimization of dropping ref-count and exception frame, 
if the compiler can prove it to be save (e.g.  no calls to any other 
code...). This can happen completely independent of either the present 
or meaning of const.
3) Add safety checks (similar to range checks)

1)
The only thing I can see, is that future optimization, added to 
const-param (of all types, e.g also records, etc) can introduce 
problems, when the value of the const param is indirectly changed, and 
that may lead to further requests of what const should or should not do. 
It may be needed to draw a line, and point out that const will always 
have some dangers. (Apparently with such a "ignore-it-fix" not any more 
for ansistrings (copy on write), but still for dyn arrays (also ref 
counted)).
Anyway if the extend off that switch is well defined, it may be worth 
having. IMHO better as compiler directive, that would allow to put it 
into the code, and make sure behaviour is stabel (rather than behaviour 
might change, if a compiler switch is used)

2)
Depends, on someone writing and contributing it. Mostly unrelated to the 
discussion.

3)
Might not only help to fix issues, but might also create awareness.

>> However, looking at the generated assembly, I can't believe there 
>> isn't a more efficient way to handle the implicit try-finally. It 
>> feels to me like that is where the problem is. There is a lot of code 
>> and several function calls that go into implementing that implicit 
>> 'finally' block. There's got to be some way to decrement the refcount 
>> even when there are exceptions but without so much overhead. It would 
>> require changes to the exception handling mechanism though. Here my 
>> knowledge runs out.
>
> The main problem here is that FPC's exception handling is based on 
> setjump/longjump. This technique has a relatively high overhead for 
> "try", but low overhead when an exception actually occurs (of course, 
> since exceptions are supposed to happen only exceptionally, that's not 
> a really good selling point). The main reason we use it is because 
> it's easy to implement.
>

For implicit exception (so long as there purpose is only to decrease 
ref-counts (and free the data if 0 reached), there is another solution.

If an exception frame already exists (even if on a higher stackframe), 
and the compiler can detect that it does exist:
then instead of creating an exception frame for the current stackframe, 
all the variables that need refcount decrement, can be added to a list. 
This list can be processed by the exception handler, before any other 
code executes. The draw back is, that memory needs to be allocated for 
this list.