Project

General

Profile

Bug #9996

Object references should be decremented at the nearest block instead of the top-level block

Added by Dănuț Filimon about 1 year ago. Updated about 1 year ago.

Status:
Test
Priority:
Normal
Assignee:
Dănuț Filimon
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
trunk/15998
reviewer:
Constantin Asofiei
production:
No
env_name:
topics:

History

#2 Updated by Dănuț Filimon about 1 year ago

  • % Done changed from 0 to 90
  • Status changed from New to WIP
  • Assignee set to Dănuț Filimon

The problem was discussed in #9538-153 - #9538-158, the problem is that we increase the reference of an object when we assign it, but also when the function assigns the return value and this can cause multiple references to be kept longer until the top-level block is executed. The solution is to decrement the reference at the nearest block.

The fix is available in 9538d, it was reviewed but during testing I found an issue (#9538-167) which takes a while to reproduce.

#3 Updated by Dănuț Filimon about 1 year ago

I focused on finding out the test suite and ran a few tests with and without 9538d on the problematic module:
  • With 7156e/15755, there are 7247 increment and decrement calls for the problematic object.
  • With 7156e/15755 + 9538d, there are 7249 increment and decrement calls for the problematic object.

The answer should lie into the additional 4 calls, currently investigating.

#4 Updated by Dănuț Filimon about 1 year ago

Dănuț Filimon wrote:

I focused on finding out the test suite and ran a few tests with and without 9538d on the problematic module:
  • With 7156e/15755, there are 7247 increment and decrement calls for the problematic object.
  • With 7156e/15755 + 9538d, there are 7249 increment and decrement calls for the problematic object.

The answer should lie into the additional 4 calls, currently investigating.

I managed to find the problematic test suite by comparing the two results, the test suite only has 11 tests and I am able to reproduce the issue much faster now :). In this scenario I tested:
  • With 7156e/15755, there are 105 increment and decrement calls for the problematic object.
  • With 7156e/15755 + 9538d, there are 105 increment and decrement calls for the problematic object. 71 of the decrement calls in ObjectOps$WorkArea.scopeFinished() are resulted from internalDefinitions. The problem lies in a DECREMENT call that is called at the end of the test, while it should be called much earlier. There is also an error in this suite that might contribute to this scenario.

#5 Updated by Dănuț Filimon about 1 year ago

  • Status changed from WIP to Review
  • % Done changed from 90 to 100
  • reviewer Constantin Asofiei added

The issue was that I was introducing the internal definitions in an external procedure, when it already has extProgDefinitions. I fixed this issue in 9538d/15910.

Constantin, please review.

#6 Updated by Constantin Asofiei about 1 year ago

Danut, some notes:
  • now we register ObjectOps as 'block-scopeable' instead of 'top-level scopeable'.
  • this means that in ObjectOps.scopeStart and ObjectOps.scopeFinished, all code which is not (yet) in a 'is this a top-level block' condition, must be moved into one. The only thing that is done for all blocks is the internalDefinitions management
  • the change for EXTERNAL_PROC - I don't understand this one. Can you please explain it a little?

#7 Updated by Dănuț Filimon about 1 year ago

  • Status changed from Review to WIP
  • % Done changed from 100 to 90

Constantin Asofiei wrote:

Danut, some notes:
  • now we register ObjectOps as 'block-scopeable' instead of 'top-level scopeable'.
  • this means that in ObjectOps.scopeStart and ObjectOps.scopeFinished, all code which is not (yet) in a 'is this a top-level block' condition, must be moved into one. The only thing that is done for all blocks is the internalDefinitions management

Ooh, you are right. So I'd have to make sure that I do not clear the objects in blocks that are not top-level (and the rest of the code that is not related to the internal definitions). I'll fix that.

  • the change for EXTERNAL_PROC - I don't understand this one. Can you please explain it a little?

The problem was that when running unit tests we would push an external procedure in UnitTestServer and any objects defined in an internal procedure (the executed test) would be registered in the internal definitions and when the test finished we didn't have any open buffers, thus the stack trace from #9538-167. I came to the conclusion that we have definitions which should have been registered in the external definitions collection which ended up in the internal definitions collection and corrected that.

#8 Updated by Dănuț Filimon about 1 year ago

  • % Done changed from 90 to 100
  • Status changed from WIP to Review

I committed 9538d/15911, now scopeStart and scopeFinished will handle internalDefinitions for all blocks and the rest of the code will only be executed for top level blocks.

Constantin, please review.

#9 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Review to Internal Test

Dănuț Filimon wrote:

Constantin, please review.

I'm OK with the changes.

I think we need rebase and after that it can go into testing.

#10 Updated by Dănuț Filimon about 1 year ago

I rebased 9538d to latest trunk/15960, the branch is now at revision 15965.

I will create a test plan and email everyone involved.

#11 Updated by Dănuț Filimon about 1 year ago

I rebased 9538d to latest trunk/15980, the branch is now at revision 15985.

#12 Updated by Dănuț Filimon about 1 year ago

9538d passed all regression tests and can be merged.

#13 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Internal Test to Merge Pending

9538d can be merged now.

#14 Updated by Dănuț Filimon about 1 year ago

  • Status changed from Merge Pending to Test
  • version_resolved set to trunk/15998

Branch 9538d was merged to trunk revision 15998 and archived.

Also available in: Atom PDF