RFR: 8328085: C2: Use after free in PhaseChaitin::Register_Allocate()

[Tobias Hartmann]

On Mon, 18 Nov 2024 10:53:41 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

> This change removes the ResourceMark from `PhaseChaitin::merge_multidefs()` because it frees memory that is used in the caller method `PhaseChaitin::Register_Allocate`.
> [My comment](https://bugs.openjdk.org/browse/JDK-8328085?focusedId=14723086&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14723086) on the JBS item explains the details.
> 
> #### Testing
> I was able to reproduce the issue on ppc64le but not on x86_64 running applications/ctw/modules/java_desktop.java. The issue didn't reproduce with this pr.
> 
> #### ResourceArea Sizes
> 
> I've traced maximum ResourceArea size after returning from `PhaseChaitin::merge_multidefs()` (see [first commit](https://github.com/openjdk/jdk/pull/22200/commits/ffbe6dee05a5a66c2965f4ff7e4cd466605cf89d)).
> I haven't found a significant difference.
> Below you can see the last trace line from each run.
> 
> ##### x86_64: 3 Runs Dacapo Tomcat 5 Iterations
> 
> ###### Baseline
> Run 1: [24.222s][info][newcode] New maximum for resource area size: 3274 KB
> Run 2: [21.317s][info][newcode] New maximum for resource area size: 3274 KB
> Run 3: [37.400s][info][newcode] New maximum for resource area size: 3336 KB
> 
> ###### PR
> Run 1: [35.002s][info][newcode] New maximum for resource area size: 3363 KB
> Run 2: [21.332s][info][newcode] New maximum for resource area size: 3274 KB
> Run 3: [36.050s][info][newcode] New maximum for resource area size: 3286 KB
> 
> ##### x86_64: 3 Runs applications/ctw/modules/java_desktop.java
> 
> ###### Baseline
> Run 1: [29.876s][info][newcode] New maximum for resource area size: 3143 KB
> Run 2: [29.631s][info][newcode] New maximum for resource area size: 3111 KB
> Run 3: [29.227s][info][newcode] New maximum for resource area size: 3142 KB
> 
> ###### PR
> Run 1: [29.755s][info][newcode] New maximum for resource area size: 3175 KB
> Run 2: [28.964s][info][newcode] New maximum for resource area size: 3143 KB
> Run 3: [28.863s][info][newcode] New maximum for resource area size: 3143 KB
> 
> ##### PPC: 3 Runs Dacapo Tomcat 5 Iterations
> 
> ###### Baseline
> Run 1: [20.041s][info][newcode] New maximum for resource area size: 3474 KB
> Run 2: [20.581s][info][newcode] New maximum for resource area size: 3474 KB
> Run 3: [20.367s][info][newcode] New maximum for resource area size: 3474 KB
> 
> ###### PR
> Run 1: [20.520s][info][newcode] New maximum for resource area size: 3506 KB
> Run 2: [20.918s][info][newcode] New maximum for resource area size: 3506 KB
> Run 3: [20.994s][info][newcode] New maximum for resource area size: 3505 KB
> 
> ##### PPC: 3 Runs ...

Nice analysis! The fix looks reasonable to me but I'm a bit worried that such removals of ResourceMarks will lead to an increase in peak memory consumption because memory is only released much later now. And I would assume there is a reason for the ResourceMark placement, i.e., below code doing significant temporary allocations. Kind of related: [JDK-8337015](https://bugs.openjdk.org/browse/JDK-8337015).

-------------

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22200#pullrequestreview-2448215994