FWD » Core Development » Base Language

I'm working on a simple approach for now. Rather than reimplementing all of the flow control for the cases where there are TM scopes, I am detecting RETURN statements in all blocks that have no TM scope or which are directly in the top-level function scope. The changes are almost ready, but I'm wondering about methods that return a value. I think this should work for them as well, they are just a variant of BlockType.FUNCTION.

Constantin: Am I forgetting something?

Greg Shah wrote:

Constantin: Please review 7045a revision 14479.

Isn't BlockManager.returnNormal supposed to throw the ReturnUnwindException? I mean, this code should distinguish between a returnNormal and a storeReturn call, as returnNormal is still emitted in cases when it originates from an inner block:

      // normal return from functions will emit in converted code with an actual Java return, we can exit
      // here without throwing the exception because the Java return will cause the control flow to move
      // back to BlockManager.function() without any of the expensive throw/catch processing for the
      // ReturnUnwindException; this should work for functions, OO property getters and methods that return
      // a value
      if (rtype == ReturnType.NORMAL && wa.tm.getBlockType() == BlockType.FUNCTION)
      {
         return;
      }

Isn't BlockManager.returnNormal supposed to throw the ReturnUnwindException? I mean, this code should distinguish between a returnNormal and a storeReturn call, as returnNormal is still emitted in cases when it originates from an inner block:

At this time, the only reason for the storeReturnValue() call is to make the converted code more readable. Anything in some nested code that has no properties will look like this:

if ...
{
   ...
   storeReturnValue(nnn);
   return;
}

instead of this:

if ...
{
   ...
   returnNormal(nnn);
   return;
}

The second form seemed confusing. Perhaps at some point we will implement different logic for the store but for now it is the same.

My point is about this code:

   public integer func2(final integer _num)
   {
      integer num = TypeFactory.initInput(_num);

      return function(this, "func2", integer.class, new Block((Body) () -> 
      {
         repeat("loopLabel0", new Block((Body) () -> 
         {
            returnNormal(0);
         }));
      }));
   }

It does exit. I've tested it here. In this case the current block type is NOT BlockType.FUNCTION so it will throw the exception.

I use wa.tm.getBlockType() to check the current block instead of looking at nearest which would be BlockType.FUNCTION for all nested blocks in that top level function.

Greg Shah wrote:

I use wa.tm.getBlockType() to check the current block instead of looking at nearest which would be BlockType.FUNCTION for all nested blocks in that top level function.

Thanks, it makes sense now.

I haven't ported this to 6129b but it should apply pretty easily. Please try it on the customer app to see the difference.

Constantin: Do you want me to merge this into 6129b? It does require a reconversion, but I'd like to get the performance benefits into that branch.

Greg Shah wrote:

Constantin: Do you want me to merge this into 6129b? It does require a reconversion, but I'd like to get the performance benefits into that branch.

I was planning to wait for the conversion to finish (tomorrow morning), and after that commit this to 6129b. The 7045a branch will be merged to trunk.

OK, go ahead when it makes sense.

There was a regression, in some cases like:

function func0 returns int.
   def var i as int.
   def var hi as handle.

   i = i + 1.
   return i.

   finally:
      delete object hi no-error.
   end.
end.

         return;
         silent(() -> 
         {
         });

I'm reconverting again the fix for 6129b. If this passes, I'll commit it, but 7045a needs some conversion testing before merging to trunk.

The problem described in #7045-15 is fixed in 7045a rev 14480.

Code Review Task Branch 7045a Revision 14480

Weird. I don't understand the reason this works, but the code seems safe enough.

7045a/14481 added another condition to emit a real Java return: the 4GL return statement must not have any downstream siblings, for cases like 'if true then return.', where FWD optimizes this to just 'return.'.

The changes from 7045a are ported and committed to 6129b/14375.

Code Review Task Branch 7045a Revision 14481

I don't object to the change, but I'm unsure if it will work quite right without changes to convert/control_flow.rules. This code:

            <!-- if marked as such, emit a real Java return AFTER the block manager call, BUT ONLY IF we are
                 not the last statement (copy.parent.numRightSiblings == 0) which is also directly in the
                 function          : FUNCTION/BLOCK/STATEMENT/KW_RETURN                   or
                 OO property getter: DEFINE_PROPERTY/KW_GET/BLOCK/STATEMENT/KW_RETURN     or
                 method            : METHOD_DEF/BLOCK/STATEMENT/KW_RETURN
                 -->
            <rule>
               copy.isAnnotation("real_return") and getNoteBoolean("real_return") and
               (copy.parent.numRightSiblings != 0 or
                (copy.parent.parent.parent.type != prog.function                 and
                 copy.parent.parent.parent.parent.type != prog.define_property   and
                 copy.parent.parent.parent.type != prog.method_def))
               <action>createJavaAst(java.kw_return, "", retParId)</action> 
            </rule>

was an attempt to implement the same concept, except I didn't handle the FINALLY block case. Don't we need to handle that in this code?

FYI, I added a history entry in rev 14482.

The code in control_flow.rules considers the case where the 4GL return. statement is the last one in the top-level block - in this case, there is no need to emit a Java return, as the block will exit anyway, naturally.

What I mention in my case is something like this:

function func0 returns int.
   if true then return 1.

   message "unreachable".
end.

   storeReturnValue(1);
   return;
   message("unreachable").

This is uncompilable code in Java, as after a return; statement, there can't be any unreachable code.

So, if we find that there is code on the 'right-side' of the 4GL return. statement, then a returnNormal(1); is emitted, which will throw a ReturnUnwindException.

I understand. My concern is not with your change, it is with my code in convert/control_flow.rules. It doesn't consider the FINALLY case, so there are places where we will emit a return; when it is not really needed. This is just a cleanup item, it isn't functional.

In #7142, we found that p2j.oo code still relies on returnNormal to throw a ReturnUnwindException. A fix is in 7142a which will end up in trunk.

Greg: for a permanent solution, we still need to refactor the p2j.oo code, to use the new storeReturnValue and Java return approach (and #7122 to avoid using BlockManager wrappers for plain getter/setter). But, I don't like changing the semantics of returnNormal, depending from where it is being called - as this can be prone to errors with hand-written code. Maybe we can add a new API which doesn't throw a ReturnUnwindException and gets emitted by the conversion rules?

But, I don't like changing the semantics of returnNormal, depending from where it is being called - as this can be prone to errors with hand-written code.

We can move the core logic from returnNormal into a worker and call it from both returnNormal and storeReturnValue. I don't want to duplicate logic.

Greg Shah wrote:

But, I don't like changing the semantics of returnNormal, depending from where it is being called - as this can be prone to errors with hand-written code.

We can move the core logic from returnNormal into a worker and call it from both returnNormal and storeReturnValue. I don't want to duplicate logic.

Yes, this can work.

Something else: a return. (or even return "something.") in an internal-procedure which is not nested in inner TM blocks, can still use the same logic as storeReturnValue.

There is another case which needs to convert to storeReturnValue:

function func0 returns int.
   return 0.

   catch ex as progress.lang.error:
      return 1.
   end.
end.

This was committed to 7300b rev 14655

• from within p2j.oo code, which forces a Java throw ReturnUnwindException: {FUNCTION=493}
• from application code, uses 'storeReturnValue' or otherwise determines that a Java return is possible: {CATCH=4, FINALLY=191, FUNCTION=149863}
• from application code, FWD uses a Java throw ReturnUnwindException: {FOR_LOOP=81, FOR_BLOCK=29, CATCH=105, INTERNAL_PROC=1226, DO=335, DO_TO=105, REPEAT_WHILE=212, EXTERNAL_PROC=146}

I think we can enhance the storeReturnValue for EXTERNAL_PROC and INTERNAL_PROC, too.

Although there is some "low hanging fruit" left to pick, when we consider how expensive it is to use Java exceptions for flow control, I think it is well worth the time to consider a more complete solution. The core problem is that by using lambdas for nested blocks of code, there is no way to return from an enclosing lambda using only a return statement in the nested/enclosed lambda.

There is a way to do this, but there is a cost. We will have to emit extra code into the converted source. The idea is that after any nested block we will need a statement that checks state and returns if "unwinding" is needed. In order to ensure we do not add runtime overhead, this should be implemented in a way that minimizes extra calls to the TM or BM, especially ones that would have to lookup context.

Please note that this is not just a solution for RETURN processing (in which we use the ReturnUnwindException) but it can also be used for LEAVE via LeaveUnwindException.

I would hope that we can also use this for the NEXT via NextUnwindException and for RETRY processing with RetryUnwindException though that will take some extra consideration because the processing of NEXT and RETRY is already highly managed inside the TM and BM.

I don't think we need to address SilentUnwindException which is supposed to only be used for an abnormal session-level exit. Thus it is not for normal flow control and is not (or should not be) performance sensitive.

We can delete ErrorUnwindException since that class is dead code (no longer used).

From a large app, NEXT is being called some ~2000 times and LEAVE some ~1000 times. In contrast, there are ~20k of QueryOffEndException (emitted when a query.next() has no record and needs to terminate the loop) and ~10k ErrorConditionException.

Constantin Asofiei wrote:

From a large app, NEXT is being called some ~2000 times and LEAVE some ~1000 times. In contrast, there are ~20k of QueryOffEndException (emitted when a query.next() has no record and needs to terminate the loop) and ~10k ErrorConditionException.

QueryOffEndException seems like a good potential. It is only raised in 13 locations and caught in 34 spots. I think the use of exceptions was a convenience to allow this to be raised deeper in the code without having to design a mechanism to return back through all the layers between these locations and the converted code where we need to honor the off end. Eliminating tens of thousands of exception uses will make a big difference. Itts time to resolve this one.

ErrorConditionException is different as it truly is an exceptional issue rather than flow of control.

Greg Shah wrote:

• Insert code after each persistence call in the converted code which can generate QueryOffEndException. That code would detect that an "off-end condition/event" had occurred and will return from that block. For example, in a FOR EACH where the query0.next() call would generate the QueryOffEndException, we might rewrite our converted code to be if (query0.next()) return;.

This makes a lot of sense. This is closer to the very-tested JDBC where .next returns a boolean.

• Eliminate the generation of QueryOffEndException and rework the call tree to return back the "off-end" state as part of the normal control flow.

Make it return false until the converted code is reached + make the container block aware that the next failed. Ok!

• Eliminate any dependencies we have on off-end listener processing and query reset processing (see TransactionManager and BlockManager for all references to QueryOffEndException). The replacement of this event processing that the query infrastructure depends upon is not as obvious to me. All I know is I want it gone. ;)

I will need some time to investigate this.

I analyzed the problem and understand most of the issue. I also have designed a solution here. However, I still have 1 single decision I am still in-doubt of, so I am looking for some advice:

1. rewrite the code as if (!q.next()) return; and still use a listener for all initialized queries inside Init, so that we can detect which queries were used for iteration. Thus, we can check at each iteration if the query reached off-end, so we avoid iterating again.
   • Good: we don't alter the code by a lot (just add an if conditional and a return); most of the work is done at run-time to infer the iterating query
   • Bad: we do extra work to infer the query using listeners at initialization stage.
2. introduce a new lambda block between Init and Body, namely Iter. This will be a boolean supplier. If this boolean supplier returns false, the query reached off-end so we stop iterating.
   • Good: we separate concerns. The iteration of the query is in the Init block and the body is in the Body block. Also, we don't do any work at run-time; the conversion already helps us with the detection of the off-end.
   • Bad: we use another lambda-function. AFAIK, lately we are trying to get rid of these. Also, such change is quite invasive into the FWD API (BlockManager).

introduce a new lambda block between Init and Body, namely Iter. This will be a boolean supplier. If this boolean supplier returns false, the query reached off-end so we stop iterating.

Is the idea that this is a callback from the query to the converted code to notify that off-end has occurred?

What does the code look like that is in this lambda?

• add a P2JQuery.hasNext() method
• pass the P2JQuery instance to the BlockManager.forEach APIs

This avoids both the lambda and the if (!q.next()) return; change. But I don't know how we should handle FOR FIRST and FOR LAST.

FOR FIRST and FOR LAST get converted to forBlock. I think it should work by injecting the P2JQuery instance just before the Block argument (or just before the label argument). All BlockManager.forEach* and BlockManager.forBlock* APIs will need to have this change.

Constantin Asofiei wrote:

There is another approach: in a FOR EACH block, is the navigation always done via query.next()? If yes, we can:

• add a P2JQuery.hasNext() method
• pass the P2JQuery instance to the BlockManager.forEach APIs

This avoids both the lambda and the if (!q.next()) return; change. But I don't know how we should handle FOR FIRST and FOR LAST.

That is right. It is the cut-off solution of having Iter if we know that q.next() is always the outcome. However, I think CompoundQuery has q.iterate(), so we have some kind of conditional here. There seems to be some accumulator logic related to the difference between next and iterate, but I am not sure what it implies (see QuerryWrapper.addAccumulator javadoc).

Greg Shah wrote:

introduce a new lambda block between Init and Body, namely Iter. This will be a boolean supplier. If this boolean supplier returns false, the query reached off-end so we stop iterating.

What does the code look like that is in this lambda?

forEach(TransactionType.FULL, "loopLabel0", new Block((Init) () -> 
{
   query0.initialize(pt, ((String) null), null, "pt.f1 asc");
}, 
(Iter) () -> 
   query0.next() // or iterate for compound query
(Body) () -> 
{
   pt.deleteRecord();
}));

This was the code I was thinking about. The Iter lambda returns he result of next.

Constantin,

forEach(TransactionType.FULL, "loopLabel0", query0, new Block((Init) () -> 
{
   query0.initialize(pt, ((String) null), null, "pt.f1 asc");
},
(Body) () -> 
{
   pt.deleteRecord();
}));

Conclusion: IMHO, Constantin's solution looks clean, so I will start working around it. I am starting to cut-out QueryOffEndException and adapt the conversion.

Constantin's solution looks clean, so I will start working around it.

Agreed. Go ahead.

Created 7045b.

Committed conversion changes to 7045b/rev. 14678. This is still a prototype as there is no run-time support for the changes. My conversion tests pass - the query is generated as the first FOR block parameter, except the cases where the first parameter is the transaction support. In this case, the query instance is referred through the second parameter (before label or enclosed list of unmanaged labels).

The only impediment I have now is the FOR FIRST case which converts as a RAQ. The first statement in the block is q.first(). Now that such iterating statement is removed, the block is unaware of the iteration type, as we presumed that this is always NEXT (#7045-41, #7045-42). In fact, only forBlock should be aware of the iteration type, as for any forEach it is correct to presume NEXT as the default iteration type. Maybe we shall generate a second parameter QueryConstants.FIRST for forBlock to support this. This also applies to LAST.

Now that such iterating statement is removed, the block is unaware of the iteration type

I think I understand your point. I agree a constant for the FOR BLOCK TYPE" is appropriate. Let's avoid the term "iterating" or "iteration" here. It is confusing for the FOR block cases since they don't iterate.

There is another approach: in a FOR EACH block, is the navigation always done via query.next()? If yes, we can:
add a P2JQuery.hasNext() method

Constantin, is this a hard constraint? I feel like making P2JQuery.next return boolean is the solution here. I agree that the classic Java iterator is based on hasNext and next, but JDBC allows only next returning boolean. I think the refactoring process would be huge if we introduce hasNext, nevertheless to mention the performance decrease. We can't easily implement hasNext without actually using next on the ResultSet. If we do so, we are basically moving the cursor using hasNext - which is not right. I am doing my implementation right now with boolean next(); let me know if you have a feedback on this.

Alexandru Lungu wrote:

There is another approach: in a FOR EACH block, is the navigation always done via query.next()? If yes, we can:
add a P2JQuery.hasNext() method

Constantin, is this a hard constraint? I feel like making P2JQuery.next return boolean is the solution here. I agree that the classic Java iterator is based on hasNext and next, but JDBC allows only next returning boolean. I think the refactoring process would be huge if we introduce hasNext, nevertheless to mention the performance decrease. We can't easily implement hasNext without actually using next on the ResultSet. If we do so, we are basically moving the cursor using hasNext - which is not right. I am doing my implementation right now with boolean next(); let me know if you have a feedback on this.

Hm... I didn't consider this; I thought there is some way to realize that there are more records without advancing. This would mean the query.next() and query.iterate() will be moved from the converted code to BlockManager.forEach blocks, which IMO is cleaner anyway.

I've continued working on 7045b. I mostly covered some issues with CompoundQuery. Currently, I am still facing some issues with CompoundQuery and FindQuery that I am addressing right now. I am still looking forward to extend the suite of test with some Presort queries and extensively test: multi-table queries with OUTER, FIRST and LAST.

EDIT: PreselectQuery and AdaptiveQuery were fully tested and there are no issues. I used OPEN QUERY and FOR EACH constructs to test these (SCROLLING and NON-SCROLLING, using FIRST, PREV, CURRENT, NEXT and LAST)

The biggest concern now is the code:

message "=== Preselect REPEAT ===".
do i = 1 to 2 on stop undo, leave:
    message "Try" i.
    repeat preselect each tt:
        find next tt.
        message tt.f1.
    end.
    message available(tt).
end.

This is generated as query.next() inside a repeat block. Currently, I modified conversion only for forEach and forBlock. I don't think it is right to include the query into the repeat block. I mean, it sounds right for forEach and forBlock as they are query related anyway. But for repeat, the PRESELECT clause is optional. That is why the technique in #7045-47 looks weird now. I am still thinking about this scenario - currently it converts, but runs infinitely.

There is a problem with current implementation on 7045b.

def temp-table tb1 field f as int field g as int.
def temp-table tb2 field f as int.
form tb1.f tb1.g tb2.f with down frame f.

def var i as int.
def var j as int no-undo.

do i = 1 to 5 transaction:
   create tb1.
   tb1.f = i.
   tb1.g = i.
end.

i = 11.
for each tb1:
   j = j + 1.
   message j.

   display tb1 with down frame f.
   down with frame f.

   tb1.f = 20.
   i = 44.

   find next tb2.
   message "found".
end.

message "i = " i.
message "tb1 avail = " available(tb1).
if available(tb1) then message "tb1.f = " tb1.f "tb1.g = " tb1.g.
pause.

clear frame f all.
for each tb1:
   display tb1 with down frame f.
   down with frame f.
end.

In this example tb2 is an empty temporary table. And we are iterating over tb1 with a for each block with default on endkey undo, leave. Inside this for block we call find next tb2, which should rise an endkey error in 4gl and leave the for block.
However with the current code on 7045b, for block lasts to the the last record in tb1, without leaving the for block on fisrt record.

Eduard, 7045b is in a "protoype" stage. There may be multiple such issues with it. In fact, the (first) implementation was just finished. It requires a second iteration of the changes and testing. Please do a step by step fixing of this issues and post only a final report of what have you fixed.

This is not a usual task that attempts to fix something. Is a task that attempts to refactor code without breaking. Currently, it is still in an intermediary stage. No need to report each issue on the way, as there may be many (and related). Keep in mind that if it wasn't working before the changes, it shouldn't be fixed now asap (eventually an issue can be opened afterwards). The goal here is to have a very similar flow of things, but with RETURN TRUE|FALSE instead of QueryOffEndException.

Please work with bzr diff to check if the changes were inaccurate and attempt to fix only parts that were touched in the process of refactoring. Maybe you will need to rollback or improve the changes in some parts. For example, in #7045-51, you shall check how it was working previously (throwing QOE) and why it is not working now (returning at the right time, the right values).

During some investigation I came across QueryConstants.RETRIEVE_MODES used in CompoundQuery.

I tried to call last on open query q3 for each tt, each tt2 where tt.f1 = tt2.f2. query, however in fwd previous is called instead of last. This is due to the use of RETRIEVE_MODES matrix. I have to mention that is the above mentioned query is optimized into a join query.

Can someone provide some assistance, why QueryConstants.RETRIEVE_MODES is used?

Eduard, please make a separate issue for #7045-53 if this can be reproduced without 7045b changes. We need to focus on having an identical solution, but without exceptions. Latent correctness concerns can be handled in other tasks.

Also, please make an update on the plan in #7045-50. What is the current status of testing? Are there other consistent changes done? (if yes, maybe prepare an intermittent commit).
In the current state of affairs, are we ready to attempt a conversion + testing on Hotel GUI for a start?

Committed on 7045b, revision 14681.

Because there are cases where an error still should be throw, I reintroduced QueryOffEnd exception. For example a find first inside a for block with on error statement.
So for now, only queries that are attached to a for block, does not throw any QueryOffEnd exceptions at all. I introduced a flag to determine if a query is attached to a for block.

I tested my changes on a few cases (for each, for first, for last), on define query construct(with scrolling and non-scrolling).
Simple multi-table joins works fine.

CompoundQuery with optimized query seems to cause quite a few issues.

Committed on 7045b, revision 14682.

For now QueryOffEnd exception is not throw in case a query is part of a for block and it exits the for block in case (query.next(), query.iterate(), query.first() or query.last()) return false;

I think we could also not throw QueryOffEnd exception, for queries defined with open query construct and also for find queries with no-error attribute.

However, I think it makes sense for find queries without no-error, to throw an exception. for, do, repeat blocks catches the error of find query inside a block and execute the on error statement.

For example, here tb2 has no element and throws a error, and leave is executed:

for each tb1 on error, leave:
   j = j + 1.
   message j.

   find next tb2.
   message "found".
end.

Committed on 7045b, revision 14683.
Also tested changes to forBlock, forEach blocks and also to next, last, first, last, unique methods logic on majic_regression_tests project. And 100% of testes pass with the current version of 7045b.

Eric: Please review.

Eric: Please review.

The changes are quite extensive, so the review will take a bit :) I am converting a big application with 7045b right now and keep you updated with the results. I am also signing up for the review process once my conversion ends and there are no problems.

PS: good news, a large customer application converted (~17 hours) and compiled (~1 hour) correctly. Also, it passed start-up (including appservers), but it failed at run-time due to #7045-62.

Eduard, I see you fixed this (#7045-62) in a new revision. I will need to recompile the project I use for test in order to pick up the new API definition (for current). I will keep you updated with the results with the updated version.

Please continue the work on statically reviewing the changes made in 7045b (a.k.a make a diff with all changes and try to spot any obvious problems). Even if the tests pass, we need to conceptually be sound with this solution - agree that we didn't miss some weird testcase in the process. Mind that the CompoundQuery issue could have been spotted easily from a diff.

The NPE is fixed now, but the regression tests I have still fail. Again, I don't have a replicate (just that the expected output of the tests is not matching). I had to combine with another branch to have my tests going; hopefully it is not due to the local rebase I made.

Eduard, lets start a full iteration of the results to make sure we are not missing obvious mistakes. I will do the same thing in parallel (there are a lot of changes). If we can't find anything valuable, I will dig into the failing test.

P.S. please look into errorIfNull flag. This will save us from detecting cases where we need / we don't need to generate QueryOffEndException.

Alexandru Lungu wrote:

I've reviewed the changes. I intend to change several things, but I will list them here for further advice:

• What is the purpose of isInsideBlock? Is this to mark that the query is used for a for iteration? I think we can remove it as part of my feedback regarding my next comment. Also, adding it as a parameter for setRecord is a bit of an overhead. Please note the widespread errorIfNull flag. We either honor it or remove it.

I used just an auxiliar data member to know if a query was defined inside a forBlock.
To fulfill the goal of not throwing QueryOffEnd exception in more cases, I think we sholud use lenientOffEnd flag, as errorIfNull is used to throw FIND First/Last not found exception in find first/last buffer statement.

• I still want to get rid of QueryOffEndException for AbstractQuery implementations. I think the QOE should be thrown only on FIND scenarios; but the FIND is never implemented with AdaptiveQuery, PreselectQuery, CompoundQuery etc.. Thus, I don't think it is right to have such pattern:

I completely removed QueryOffEnd from CompoundQuery.
However for AdaptiveQuery, PreselectQuery there is a catch, find next tt gets converted into an AdaptiveFind, and next method of AdativeFind calls next method of its parent class AdaptiveQuery. So I think it is not very safe for now to completly remove catch (QueryOffEnd) from AdaptiveQuery and PreselectQuery.

I think it is not right to have both available and QueryOffEndException. Eduard, can you clarify in which cases we need to throw QOE as an error condition to the user, beside FIND? I will like to attempt something like: never QueryOffEndException except cases where absolutely necessary instead of always use QueryOffEndException except cases where we iterate.

I think find first/last tt are cases when error dialog appears with message FIND First/Last Faild for buffer tt.
Or find next/prev tt where just a exception is thrown.

• In CompoundQuery, retrieve doesn't expect to return booleans. At best, it should return DMO or DMO ids. The code is not good and is suspect to ClassCastException.The same goes for return new Object[] { query.first(lockType) };, etc. I like the Optional attempt more, as it clearly separate: no results, has result but didn't peek, has result and peeked

Change to used return query.getRow() for queries with peek = false.

Committed the changes on 7045b, revision 14686.

Eduard, mind setting up some large customer applications and do some smoke tests. I will take the changes and do on some other applications I have set-up. I am planning to do a review first thing tomorrow morning.

I rebased 7045b to latest trunk. It is now at rev. 14833.
Eduard, please double-check: RecordBuffer (especially setRecord), PreselectQuery and AdativeQuery (especially coreFetch).

EDIT: I will need to reconvert my testing application now that 7045b was rebased.

Converted and setup a large customer application using 7045b branch, and used it for a bit. Did not noticed any major issues, like application crash or noticeable regressions.

Also committed on 7045b, revision 14834.

If you think it is ready, let's get ChUI regression tests, ETF and the big POC tested.

Greg, I can do that for POC, but it will take me a day to have it converted. Also mind that I am already reconverting with #6649, so this will be delayed for another day :/

Understood. For such extensive changes to our control flow, it seems necessary.

Eduard, I didn't had the chance to convert yet the POC. Can you attempt it? I have already queued some other performance items, so I missed the timing. Thank you!

Eric: Please don't forget to do a code review ASAP.

Code review 7045b/14825-14834:

There are file header entries problems (i.e., duplicate entries in some files, missing entries in most files). For example, control_flow.rules entries 99 and 101; database_access.rules entries 122 and 125; BlockManager entries 62 and 69; etc. Please fix these. AdaptiveFind and other classes have edits but no file header entries. This list is not exhaustive. Every file that changed must have a header entry.

The javadoc comments for every query navigation method which has changed its control flow mechanics (from exceptions to boolean return value) needs to be updated to explain the new control flow. Accordingly, if QueryOffEndException is no longer thrown by these methods, there should no longer be a throws javadoc tag for that exception. Check whether class-level javadoc should be updated as well.

I have noted the discussion above that QOEE is not yet fully eliminated. I see this, for instance, with the try-catch blocks in AdaptiveQuery.first(LockType). Do we have an idea how much effort is left to fully achieve the removal of QOEE? I ask because as long as we are in this hybrid state of implementing error handling both using exceptions and return values, the code is more complex and we are asking for regressions.

P2JQuery.setLenientOffEnd(boolean) (and every override of it; e.g., QueryWrapper.setLenientOffEnd(boolean)) needs javadoc. This is especially important, since the semantic of "lenient-off-end" certainly has changed with this branch.

Why was the outer-join-specific logic starting around line 6206 of PreselectQuery changed/removed? I'm not saying it's incorrect, but I don't see an explanation anywhere. As in other places, QOEE is still caught and processed here. Is it actually thrown or does this error handling need to be refactored?

QueryConstants.ITERATE needs javadoc.

RecordBuffer.setRecord is missing javadoc for the new lenientOffEnd parameter. Should this method still be throwing QOEE?

Like the query changes, the changes to the BlockManager APIs are missing javadoc for the new parameters and an explanation of the new flow control. We really should not have logic inside BlockManager which is conditional upon query instanceof CompoundQuery. Can this be avoided and refactored into the persist package without breaking the fundamental changes we are trying to achieve with this task?

This set of changes will need considerable conversion and runtime testing.

Eduard, please make an update on your status of running the POC with your changes and addressing Eric's #7045-77.

I am addressing some regression on running POC. But I think, I am close to find the problem causing this regression. I want to make sure that I do not face any regression, and after that I will address #7045-77.

Eric Faulhaber wrote:

I have noted the discussion above that QOEE is not yet fully eliminated. I see this, for instance, with the try-catch blocks in AdaptiveQuery.first(LockType). Do we have an idea how much effort is left to fully achieve the removal of QOEE? I ask because as long as we are in this hybrid state of implementing error handling both using exceptions and return values, the code is more complex and we are asking for regressions.

Well, I think that QueryOffEnd can not be fully eliminated as there are cases when this kind exception is thrown in 4gl, and from that depends to control flow of program.
For example in case of an do block with an on error undo statement, if there is an find next tt statement inside that just returns a boolean, it will change the control flow of the program.
Basically it will iterate while the do block condition is met, when it should exit the block at the moment find next tt returned no result.
And this statement is converted in AdaptiveFind query, we can get rid of catching QQE in its parent classes AdaptiveQuery and PreselectQuery

Why was the outer-join-specific logic starting around line 6206 of PreselectQuery changed/removed? I'm not saying it's incorrect, but I don't see an explanation anywhere. As in other places, QOEE is still caught and processed here. Is it actually thrown or does this error handling need to be refactored?

Sorry, this was probably removed at rebase.

RecordBuffer.setRecord is missing javadoc for the new lenientOffEnd parameter. Should this method still be throwing QOEE?

As I said previously, there are cases where it is natural to throw such an exception, I added lenientOffEnd paramenter to check cases when this error should not be thrown. (queries define inside an for each block or using define query statement).

Like the query changes, the changes to the BlockManager APIs are missing javadoc for the new parameters and an explanation of the new flow control. We really should not have logic inside BlockManager which is conditional upon query instanceof CompoundQuery. Can this be avoided and refactored into the persist package without breaking the fundamental changes we are trying to achieve with this task?

I think this could be done easily, because iterate method of CompundQuery calls retrieveImpl with direction parameter as NEXT. So I could just leave call to query.next() inside the BlockManger.

I finally managed to run POC, converted using 7045b branch without any regressions.

Eduard, please address the review and commit a final solution to 7045b (including javadoc / history entries / etc.) and ensure it is working with POC. Afterwards, move it to 100% Done and Internal Test.

I would like to have it profiled asap and eventually merge. Can you have it done in the next couple of hours, so I have time to complete the profiling today?

• do for tt on error undo, leave:
     message tt.f1.
  end.

• do preselect each tt on error undo, leave:
     message tt.f1.
  end.

• repeat for tt on error undo, leave:
     message tt.f1.
  end.

Committed on 7045b, revision 14835.

I tested uast/query_off_end/qoe_exception , did not see any regressions. In all the cases that you mentioned query is just preselected, and an iteration is performed only programmer explicitly call an operation on that query (first, last, etc). And when query.next() does not have any record to fetch, QQE is raised in 4gl and also in fwd.

• Eduard, mind that you have some hard-tabs in your last commit (AdaptiveFind history entry and in BlockManager where you removed the iterate).
• I think the outer logic is still lost and should be added back (in PreselectQuery). This won't be noticeable in the POC tests, as 7156b disables outer-joins for PreselectQuery.
• You have places where you set referenceRecord and call accumulate only if the DMO is not null. This is not compatible with the original code (pre-7045b). For instance, in RandomAccessQuery.next, the "old" code was doing:
  

  OffEnd offEnd = (dmo == null ? OffEnd.BACK : OffEnd.NONE);
  updateBuffer(dmo, lockType, false, offEnd);
  referenceRecord = dmo;

• addResultLast and addResultFirst changes are right from my POV.

I retested performance today and it is still "slower" side.

Alexandru Lungu wrote:

I've run 7045b with the performance tests and got 8.168s. Comparing to the baseline of 7.958s, this is +2.5% slower. IMHO, last week I profiled ~5 different branches and got results from +1% to +3% on all "theoretically" faster solutions. I've rerun the baseline and still had that ~7.950s timing. I am concerned that we are missing a point here or my system is not doing great at all.

Talking strictly about 7045b, Eduard, we need some kind of statistic to see:

• how many QOE are still thrown in 7045b vs 7156b; did we had a massive reduction of such exceptions being thrown?
• further, we may need a deeper understanding on:
  • how many for-each blocks were executed and the average number of steps
  • how many for-each blocks were ended by a next returning false
  • the different query flavors: PreselectQuery vs AdaptiveQuery vs PresortQurery vs CompoundQuery used in for-each blocks.

I will review the changes to spot any obvious performance downgrade.

This is pretty strange, I tested POC with 7156 and POC using 7045b conversion, and this are some results:
- POC with 7156b, warmup and 1 test, around 40000 times QQE is thrown.
- POC with 7045b conversion, also warmup and 1 test, 1021 QQE exceptions are thrown.

I also made quite a few profilings yesterday, and this are some results:
- POC with 7156b, the most time consuming method is RecordBuffer.throwOffEnd, it takes around ~12% of time of execution of 20 performance tests.
- POC with 7045b, RecordBuffer.throwOffEnd takes considerable less time, around ~1% of the time.

Also every time I tested POC with 7045b was with at least 1s faster.

Eduard, please focus on reaching a "state-of-the-art" solution for 7045b (also referring to #7045-84). Also, triple check we didn't miss something from it.
I intend to start a (very) long running testing process with 7045b, so we need to make sure it is fully completed and the final solution tested with whatever we have available (POC, large cust. application, Majic).

Alexandru, I haven't looked at the full code, but is the QueryWrapper.handleQueryOffEnd still needed?

I think is not needed, as QueryWrapper is used in conversion of open query statement. And in this case QQE is not thrown.
I tested on POC without QueryWrapper.handleQueryOffEnd, and got no visible regressions.

Eduard, I started converting a really large application with 7045b (on top of 7156b). You have 3 days left to do the changes, before the run-time testing starts :)

Regarding your last comment: it depends upon the delegate right? If the delegate is not lenient-off-end, it may throw QOE, right? So we should ensure first that the delegates are always lenient-off-end.
Please check QueryWrapper.assign(RandomAccessQuery) - it is automatically making the query lenient-off-end. I think this can be done to the other assigned queries as well (PreselectQuery and CompoundQuery).

Alexandru Lungu wrote:

Regarding your last comment: it depends upon the delegate right? If the delegate is not lenient-off-end, it may throw QOE, right? So we should ensure first that the delegates are always lenient-off-end.
Please check QueryWrapper.assign(RandomAccessQuery) - it is automatically making the query lenient-off-end. I think this can be done to the other assigned queries as well (PreselectQuery and CompoundQuery).

In CompoundQuery, lenient-off-end is set in constructor and in retrieveImpl for every query component.
In PreselectQuery, lenient-off-end is set in open method, I quess that this method should always be called when a PreselectQuery is used as a delegate to a QueryWrapper.
But I could add setting QQE in QueryWrapper

Eduard, there is an API issue we have on BlockManager. I think the rewriting of the BlockManager API to include the query had regressed at some point. Please double check that. This is an example which currently fails:

FOR tt WHERE tt.f1 = 2 NO-LOCK. END.

Mind that this is not FOR EACH, neither FOR FIRST. It should convert to a forBlock. This is how it was converted now:

forBlock(query25, "blockLabel28", new Block((Init) () -> ...);

Before 7045b:

forBlock("blockLabel28", new Block((Init) () -> ...);

Before 7045b we had forBlock(String, Block) method in BlockManager.

In 7045b, we don't have forBlock(P2JQuery, String, Block)!

Thus, please recheck the BlockManager API and fix the problems you encounter. Make this your top-priority. I can't run the regression tests as they don't compile.

I discussed with Eduard and it seems that the problem is that we have forBlock(P2JQuery, int, String, Block), but we missed generating the direction, so we end up with forBlock(P2JQuery, String, Block). Thus, the conversion is broken.

I've implemented a work-around locally to have it compile, so I can get it running with the regression tests. However, the patch is quite "dirty" (adding back the old signatures and mapping them at run-time to the new signatures from BlockManager). I hard-coded the direction as UNIQUE when not specified. This isn't "bad", but it is clearly better to fix this at conversion to go the proper way.

I think the conversion changes to fix this will be quite safe and the occurrences of such constructs are very rare anyway. Run-time changes aren't required if we approach to fix this at conversion.

To have it tested "cleanly", I will need to reconvert and recompile (but this may take ~3/4 days). If I stick to the run-time testing now, I hope we can get some results first thing tomorrow morning (apparently, the compiling takes ~1day).

Committed on 7045b, revision 14837.

This change requires reconversion. 4gl constructs like for tt, should be converted in forBlock(query, QueryConstants.UNIQUE, ...).
Also made a change in BlockManager, to call query.unique() when direction parameter is equal to QueryConstants.UNIQUE.

I tested conversion on Hotel, but I also am planning today to run conversion on a big customer application.

Eduard, please mind that there are still some hard-tabs in the last 2 commits. (BlockManager).

7045b passes a large set of regression tests from a customer application. I tested without the changes in rev. 14837.
I was delayed a bit by some false negative tests that were due to a wrong rebase. There were 27/43 tests failing with that wrong rebase.

I retested now with everything in order (good job Eduard in catching that rebase mistake). I have 20/43 tests failing after fixing the problem. With the a baseline FWD (which I am not sure of the version), there are also 20/43 tests failing (the same ones).

However, I've done this tests with a rebased 7045b from 7156b, but before rev. 14837. I will add 14837 as a patch and retest to ensure that the latest version is OK.

Eduard, my testing is getting close to an end. Please let me know if you have something else to do on 7045b. Otherwise, we can consider it ready for merge.

Removed hard tabs from the previous commit. Committed on 7045b, revision 14838.
I do not think there is anything else to add on this branch.

Completely finished the testing of 7045b. Pending for merge.

Quick final look at 7045b/14838...

It looks like we still need a rebase to current trunk before merge. Also, some minor stuff to be fixed:

Header entry 122 was removed from database_access.rules. It was redundant with 125, but now the numbers are off.

There are still hard tabs in AdaptiveFind (line 106).

P2JQuery header entry 66 should not be numbered; it is in the same branch to be merged to trunk as entry 65. Same for QueryWrapper header entry 99; same branch as 98. Same for RandomAccessQuery entries 129 & 130. BlockManager: 70-72.

Where specific file header entry numbers are mentioned above, these numbers may of course change with the rebase.

As long as the rebase doesn't introduce any complications, none of the above changes should invalidate the testing done, so after this cleanup, we should still be ok for merge.

Just rebased 7045b to latest trunk, but the process was a bit slow due to the large number of changes to be introduced. Eduard, please do some final quick tests with this branch before merging.

Hold off on the merge. 8056a and 8127a need to go first.

Looked at 7045b after rebase, it looks fine.
Done some minor fixes at history entry.
In forBlockWorker, changed in case of direction parameter is equal QueryConstans.unique to call query.unique instead of last.
Also, comp.getQuery().setLenientOffEnd(true) in CompoundQuery.retrieveImpl was missing after rebase.

Committed on 7045b, revision 14903.

Nice catches, Eduard! Please do a second iteration of the changes - I really want to make sure we don't missed something here :)

Constantin, ETF tests are the only ones that were not run. I rebased 7045b to latest trunk and it is now at 14907. Eduard, before your final review, please do an update.

Alexandru Lungu wrote:

Constantin, ETF tests are the only ones that were not run. I rebased 7045b to latest trunk and it is now at 14907. Eduard, before your final review, please do an update.

I'll have ETF testing later tonight, need to reconvert.

Constantin, do you feel like it is better to get everything in trunk and then run ETF against all, instead of "micro" testing?

I was looking at Greg's recent email.

7045b passed ETF testing.

You can merge 7045b to trunk after 3303a.

7045b is at the front of the queue now, go ahead and merge.

See #6667-823 for the needed changes for hand coded Java in the ChUI regression test application.

Created 7045c and committed a changed regarding the removal of .next calls inside the BlockManager.forEach body. There are not needed as the block manager was already applying the .next. This problem was not seen in practice; I observed it right now. It could have cause memory leaks or even unexpected behaviors:

=== modified file 'src/com/goldencode/p2j/persist/RecordBuffer.java'
--- old/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-01-18 00:21:46 +0000
+++ new/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-01-23 11:43:10 +0000
@@ -1299,6 +1299,7 @@
 **     RAA 20231220          Fixed a case in which ReflectiveOperationException could have been thrown when
 **                           a RecordBuffer was created through a proxy.
 ** 343 OM  20240117          Removed expired comment.
+** 344 AL2 20230123          Removed unwanted .next call inside BlockManager.forEach.
 */

 /*
@@ -8104,7 +8105,6 @@
             },
             (Body) () ->
             {
-               q.next();
                try
                {
                   delete();
@@ -8168,7 +8168,6 @@
          {
             public void body()
             {
-               query.next();
                try
                {
                   delete();

Please review and let me know if I can merge.

Eric: Please review.

Alexandru, is 7045c still needed after we refactored the query management to be inside BlockManager? If not, please archive as 'dead'.

Greg: I've created 7045d from trunk rev 15041. In rev 15042, I've added support for Java return from within internal procedures. Please review.

Constantin Asofiei wrote:

Alexandru, is 7045c still needed after we refactored the query management to be inside BlockManager? If not, please archive as 'dead'.

Absolutely; 7045c attempts to remove a "hard" q.next from within the body of a BlockManager.forEach that uses the query inside already. The issue is that the next is executed twice (once inside the body and once from withing the BlockManager). I can merge 7045c independently after review or let you integrate it in 7045d.

Greg, please review 7045d - I'd like to get this into trunk, it doesn't hurt.

Code Review Task Branch 7045d Revision 15042

In return_stmts.rules, the type == prog.procedure or on line 93 seems incorrect. It will leave inFunc set to true for procedures.

Otherwise the changes are good.

Greg Shah wrote:

Code Review Task Branch 7045d Revision 15042

In return_stmts.rules, the type == prog.procedure or on line 93 seems incorrect. It will leave inFunc set to true for procedures.

Thanks, rebased and fixed in rev 15061.

I'll do another round of conversion with a small app and I'll put it in my merge queue (probably tomorrow).

Sounds good.

Greg, can this be merged? It affects a customer's app.

If you are confident it is safe, it can be merged now.

Branch 7045f was created from trunk rev 15077 - in rev 15078 re-fixed the regression in 7045d/e. Merged to trunk rev 15078 and archived.