FWD » Core Development » Base Language

Based on #9663, I've noticed that for a FIND ... NO-ERROR that is executed ~130k times with no record found, executing errorReport inside finalzeFind takes about 40%. This is because ErrorConditionException is thrown and caught multiple times.
My idea is to introduce a new flag for ErrorManager, preventThrow, that can be enabled in between errorReport.run() when silent mode is active and to return instead of *throw*ing in recordOrThrowError.

With this I expect a 5% total gain on the scenario of #9633.

Committed changes to 8273a/16020. Please review!

Understood now. Lets test it.

Review of 8273a:

• I think there is an issue with setPreventThrow bracketing. If errorReport.run(); is throwing an exception, the reset won't be done. Please embed errorHelper.setPreventThrow(false); in a finally block.
  • errorReport is usually a trivial call to ErrorManager, but any non-expected exception can cause havoc (a NPE for instance).

Concern on the current implementation:

• Please check a CompoundQuery with FOR EACH ..., FIRST ... syntax unoptimized. As the second component is a RAQ, it will be executed within the CompoundQuery
  • First table have several records, the second has none. Do a GET FIRST ..... It will find something in the first component, but within the second there will be nothing, so it should throw a Query off-end exception, so that CompoundQuery knows that there is no proper match with the second component. If the GET FIRST is run within a silent batch (NO-ERROR), the second component will think it is running a silentFirst and will not throw query off-end exception. This is theory and should be tested ... does it work as expected? Same applies to GET LAST.
  • My point is that isSilent may be set somewhere higher up the stream, so that the RAQ will think that it should avoid throwing errors, but, in fact, the SILENT was not meant for it.
• CAN-FIND and CAN-FIND also converts to a FindQuery. Can we run the CAN-FIND inside a silent-block so the RAQ within will incorrectly think that the SILENT was for it?

Overall, I think it is risky to use errorHelper.isSilent(). I would rather make use of the existing silentFirst, silentLast, etc. method to properly target your changes for these constructs. A CompoundQuery or CAN-FIND will use the usual API of first, last, etc., but only FIND ... NO-ERROR will convert to a silent* method.

Now that I am looking to the code I see inside silentFirst:

      boolean wasLenientOffEnd = isLenientOffEnd();
      if (!wasLenientOffEnd)
      {
         setLenientOffEnd(true);
      }

This is basically forcing the query to be lenient-off-end (aka not throw errors when running off-end). This flag is used on setRecord so that RecordBuffer will call RB.throwOffEnd if the DMO is null. However, this check is done only for NEXT and PREVIOUS. FIRST, UNIQUE and LAST use finalizeFind as you noted.

• with 8273a, isSilent is used and may represent a relaxed condition. We should drive the supressing by setting a flag in silentUnique, silentFirst and silentLast.
  • double-check how NEXT and PREVIOUS work with silent*. They do not use finalizeFind.
• with trunk, silentNext and silentPrevious may not set the error-status properly.
  • lenientOffEnd should be used internally in CompoundQuery for instance or FOR EACH blocks that do NEXT under the hood.
• ensure that the suppressing of QOEE is only for FIND ... NO-ERROR pattern and not for other embedded usages inside CAN-FIND, CompoundQuery or invalidated AdaptiveQuery.

Maybe we should unify the implementation to use lenientOffEnd, but rather rename it to offEndBehavior make it an enum: NONE, SILENT (only log), THROW. This will force us to implement the lenientOffEnd properly once and for all.
Also, I do not like that we also have a errorIfNull separate flag. Why would that be any different? Look at RecordBuffer.setRecord javadoc:

    * @param   errorIfNull
    *          {@code false} to raise an end condition if the record being set is {@code null};
    *          else {@code true} (caller is responsible for handling appropriately in the latter case).
    * @param   lenientOffEnd
    *          {@code false} to raise an end condition if the record being set is {@code null};
    *          else {@code true} (caller is responsible for handling appropriately in the latter case).

Andrei Iacob wrote:

• lenientOffEnd should be used internally in CompoundQuery for instance or FOR EACH blocks that do NEXT under the hood.

• ensure that the suppressing of QOEE is only for FIND ... NO-ERROR pattern and not for other embedded usages inside CAN-FIND, CompoundQuery or invalidated AdaptiveQuery.

I'm looking into this now.

None of these hit the changes made.

Committed the rest of the changes. Please take a look!

Review of 8273a:

The changes are good; only minor things to assess:

• The save/restore logic in RandomAccessQuery and AbstractQuery should rather save the OffEndBehavior then a boolean. It would seem more natural than actual processing that and restoring from processed behavior.
• For CompoundQuery retrieve logic, the query underneath should be OffEndBehavior.NONE. Otherwise, when reaching the off-end of a component in CQ will set the ERROR-STATUS, which is not correct. Please unit test this case.
• For PreselectQuery, when doing an OPEN, I am not sure that running off-end will set ERROR-STATUS. I tend to think that it should be OffEndBehavior.NONE. Please unit test this case.
• For QueryWrapper, are the OPEN QUERY in general OffEndBehavior.SILENT? I would expected these to be OffEndBehavior.NONE. In other words, doing GET NEXT off-end should or should not set the error status? Please unit test this case.
• I really think BlockManager should set OffEndBehavior.NONE. Definitely an off-end FOR EACH block is not going to set the ERROR-STATUS. Please unit test this case.
• Please make the save/restore logic in finalizeFind more canonic. In other words, please save the isPreventThrow and set it back. You may have nested setPreventThrow and I don't quite enjoy hard-coding the true/false when setting.

While I understand that currently OffEndBehavior.SILENT and OffEndBehavior.NONE are handled the same, this should not semantically be the case. NONE should not set the error-status. Even though your implementation doesn't ever reach that point, the semantic is confusing (some SILENT are registering the ERROR and some are not). Places that shouldn't set the ERROR should be made NONE instead of SILENT.

Alexandru Lungu wrote:

Review of 8273a:

The changes are good; only minor things to assess:

• For CompoundQuery retrieve logic, the query underneath should be OffEndBehavior.NONE. Otherwise, when reaching the off-end of a component in CQ will set the ERROR-STATUS, which is not correct. Please unit test this case.
• For PreselectQuery, when doing an OPEN, I am not sure that running off-end will set ERROR-STATUS. I tend to think that it should be OffEndBehavior.NONE. Please unit test this case.
• For QueryWrapper, are the OPEN QUERY in general OffEndBehavior.SILENT? I would expected these to be OffEndBehavior.NONE. In other words, doing GET NEXT off-end should or should not set the error status? Please unit test this case.
• I really think BlockManager should set OffEndBehavior.NONE. Definitely an off-end FOR EACH block is not going to set the ERROR-STATUS. Please unit test this case.

While I understand that currently OffEndBehavior.SILENT and OffEndBehavior.NONE are handled the same, this should not semantically be the case. NONE should not set the error-status. Even though your implementation doesn't ever reach that point, the semantic is confusing (some SILENT are registering the ERROR and some are not). Places that shouldn't set the ERROR should be made NONE instead of SILENT.

None of the above set error status, so I agree that from a semantically point of view offEndBehavior should be set to NONE.

• The save/restore logic in RandomAccessQuery and AbstractQuery should rather save the OffEndBehavior then a boolean. It would seem more natural than actual processing that and restoring from processed behavior.
• Please make the save/restore logic in finalizeFind more canonic. In other words, please save the isPreventThrow and set it back. You may have nested setPreventThrow and I don't quite enjoy hard-coding the true/false when setting.

Done. Committed changes, current revision 16032.

I am OK with the changes. Please proceed with comprehensive testing.

• majority of them are caused by RAA.prev due to introducing finalizeFind with noRecordOrThrowError. Reverting this fixes the issue;
• half of gso_rfq_tests fail - One test has a crash and the other depend on it. I'm looking into it.

Andrei Iacob wrote:

• majority of them are caused by RAA.prev due to introducing finalizeFind with noRecordOrThrowError. Reverting this fixes the issue;

This is caused by how finalizeFind for RAA.next and RAA.previous was used. In both cases, finalizeFind executed updateBuffer with errNull as isErrorIfNull(), but next and prev always sent this param as false. Alongside this, for RAA.prev, offEnd was sent as BACK instead of FRONT.

• half of gso_rfq_tests fail - One test has a crash and the other depend on it. I'm looking into it.

Following the discussion in #7143-1631 this is a regression from trunk.

Committed changes.

I am OK with the changes in 8273a.

Testing completed and passed for:

Love it! Please merge 8273a to trunk now.

Branch 8273a was merged into trunk as rev. 16112 and archived.