Allocate TreeInfo objects using TR::Region of containing List

[hzongaro]

If the findOrCreateTreeInfo function fails to find a TreeInfo object that refers to a particular TR::TreeTop in the targetTrees List, it allocates a new TreeTop instance using trStackMemory. However, findOrCreateTreeInfo is called within a call tree from DeadTreesElimination::process. The process method creates a new TR::StackMemoryRegion on entry to the method, but the targetTrees List has a lifetime that extends for the duration of the optimization. (The _targetTrees is first constructed in the DeadTreesElimination constructor and is only cleared in the prePerformOnBlocks method.

The TreeInfo objects should be allocated from the same TR::Region that the List uses for allocation of its nodes to ensure that their lifetime is as long as that of the containing List object.

Fixes: Issue eclipse-openj9/openj9#19197

[hzongaro]

@vijaysun-omr, may I ask you to review this change?

One thing that isn't absolutely clear to me is whether it's absolutely necessary to allow for the possibility that _targetTrees could contain an entry that refers to a TR::TreeTop that came from an extended block that was processed in a previous call to DeadTreesElimination::process(TR::TreeTop*,TR::TreeTop*). That's effectively what this change does.

I attempted test runs with assertions added to check for that possibility, and did not encounter any assertion failures. However, I don't know whether there's a guarantee that that situation could not arise.

I think this change is safe, but it might be that we could instead narrow the lifetime of _targetTrees so that it — and the TreeInfo objects it contains — could be allocated using the same stack memory region that's created on entry to that process method.

[vijaysun-omr]

Thanks @hzongaro I don't know of a reason why we would want to refer to a tree info for something that was processed in a prior pass of the dead trees elimination optimization (as I think was the gist of your question). This is a bit clunky since the state of the IL trees could have changed to make the "height" calculation wrong in the intervening optimizations.

So I think

I think this change is safe, but it might be that we could instead narrow the lifetime of _targetTrees so that it — and the TreeInfo objects it contains — could be allocated using the same stack memory region that's created on entry to that process method.

is an option to consider.

[hzongaro]

So I think ... is an option to consider.

Thanks, @vijaysun-omr. I've reviewed the usage of DeadTreesElimination::process(TR::TreeTop*,TR::TreeTop*) and verified that it is safe to allocate the targetTrees List in the in-scope StackMemoryRegion rather than using a longer-lived field associated with the optimization. I've pushed commit 870af4d0ce3f9e7cda3390bb13b4b3700774b48a to make that change. If you are good with it, I will squash the commits.

[vijaysun-omr]

Thanks, you can squash the commits and I can run testing after that.

[hzongaro]

Jenkins build all

[vijaysun-omr]

jenkins build riscv

[vijaysun-omr]

jenkins build win

[vijaysun-omr]

jenkins build riscv

[hzongaro]

The riscv failure does not appear to be specific to this pull request:

17:28:16  29/30 Test #29: comptest ..........................Child killed***Exception: 1509.37 sec
17:28:16  test 30
17:28:16        Start 30: compunittest
17:28:16 
...
17:28:16  
17:28:16  97% tests passed, 1 tests failed out of 30
17:28:16  
17:28:16  Total Test time (real) = 1947.83 sec
17:28:16  
17:28:16  The following tests FAILED:
17:28:16     29 - comptest (Child killed)
17:28:16  Errors while running CTest 

The same failure appears in this PR build for pull request #7315.

The x86-64 macOS failure appears to be due to known issue #7181.