Skip to content

Conversation

@gnodet
Copy link
Contributor

@gnodet gnodet commented May 22, 2025

Issues Fixed:

  1. Profile Activation Context Recording Bug: When there was a cache hit
    in readAsParentModel(), the keys accessed during cache matching re-evaluation
    were lost because the recording context was discarded without merging the
    recorded keys into the parent context.

  2. Recording Context Isolation: Parent model processing was polluting the
    child's recording context with intermediate keys that weren't essential to
    the final result, leading to noisy cache keys and reduced cache effectiveness.

  3. Profile Injection Precedence: Profile properties from distant ancestors
    were incorrectly overriding direct properties from closer ancestors due to
    improper profile injection timing during inheritance assembly.

Solutions Implemented:

  1. Cache Hit Recording Fix:

    • Create a separate recording context for cache matching re-evaluation
    • Replay cached record keys into the parent recording context on cache hits
    • Ensure all essential keys are captured regardless of cache hit/miss
  2. Recording Context Isolation:

    • Use separate recording contexts for parent model processing
    • Only replay essential keys from the final result into parent context
    • Maintain clean, precise cache keys for better performance
  3. Profile Injection Timing:

    • Inject profiles into child models using parent's profile list
    • Ensure proper precedence where child elements override parent elements
    • Maintain correct inheritance hierarchy during profile activation

Technical Details:

  • Made DefaultProfileActivationContext.Record fields package-private for merging
  • Added replayRecordIntoContext() method for proper key propagation
  • Updated profile injection to respect inheritance precedence rules
  • Enhanced MavenModelMerger to handle profile content merging correctly

Benefits:

  • Eliminates race conditions in concurrent model building scenarios
  • Improves cache effectiveness through cleaner, more precise cache keys
  • Ensures correct profile activation context propagation through inheritance
  • Maintains proper property precedence in complex inheritance hierarchies
  • Reduces memory usage by avoiding over-recording of intermediate keys

This fix ensures that profile activation behaves consistently and correctly
in both single-threaded and multi-threaded environments, while maintaining
optimal performance through improved caching mechanisms.

JIRA issue: MNG-8736

@cstamas
Copy link
Member

cstamas commented May 23, 2025

@wendigo can you verify this fix?

@wendigo
Copy link

wendigo commented May 23, 2025

@cstamas i did. Doesn't work

@wendigo
Copy link

wendigo commented May 27, 2025

❯ ./mvnw --version
Apache Maven 4.0.0-rc-4-SNAPSHOT (86496837b6b3cd5a05210941917a48427e29e38d)
Maven home: /Users/mateusz.gajewski/.m2/wrapper/dists/apache-maven-4.0.0-rc-4-SNAPSHOT/a25e2747
Java version: 24.0.1, vendor: Eclipse Adoptium, runtime: /Users/mateusz.gajewski/.sdkman/candidates/java/24.0.1-tem
Default locale: pl_PL, platform encoding: UTF-8
OS name: "mac os x", version: "15.3.1", arch: "aarch64", family: "mac"

I'm still getting wrong model from time to time

@wendigo
Copy link

wendigo commented May 27, 2025

❯ ./test.sh 8
+ for x in {1..100}
+ echo -ne '1: '
1: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'wrong model'
wrong model
+ for x in {1..100}
+ echo -ne '2: '
2: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'wrong model'
wrong model
+ for x in {1..100}
+ echo -ne '3: '
3: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '4: '
4: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '5: '
5: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '6: '
6: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '7: '
7: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '8: '
8: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '9: '
9: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '10: '
10: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'ok model'
ok model
+ for x in {1..100}
+ echo -ne '11: '
11: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class
+ echo 'wrong model'
wrong model
+ for x in {1..100}
+ echo -ne '12: '
12: + ./mvnw -B --offline -e help:effective-pom -pl :flink-loader-query-history-export-job -Dmaven.modelBuilder.parallelism=8
+ grep -q main-class

@wendigo
Copy link

wendigo commented May 27, 2025

It also fails during execution:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce (default) on project flink-loader-query-history-export-job: Execution default of goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce failed: Failed to find reference POM which defines the current value -> [Help 1]

This commit addresses multiple critical issues in the profile activation
context recording and caching mechanism during model inheritance:

## Issues Fixed:

1. **Profile Activation Context Recording Bug**: When there was a cache hit
   in readAsParentModel(), the keys accessed during cache matching re-evaluation
   were lost because the recording context was discarded without merging the
   recorded keys into the parent context.

2. **Recording Context Isolation**: Parent model processing was polluting the
   child's recording context with intermediate keys that weren't essential to
   the final result, leading to noisy cache keys and reduced cache effectiveness.

3. **Profile Injection Precedence**: Profile properties from distant ancestors
   were incorrectly overriding direct properties from closer ancestors due to
   improper profile injection timing during inheritance assembly.

## Solutions Implemented:

1. **Cache Hit Recording Fix**:
   - Create a separate recording context for cache matching re-evaluation
   - Replay cached record keys into the parent recording context on cache hits
   - Ensure all essential keys are captured regardless of cache hit/miss

2. **Recording Context Isolation**:
   - Use separate recording contexts for parent model processing
   - Only replay essential keys from the final result into parent context
   - Maintain clean, precise cache keys for better performance

3. **Profile Injection Timing**:
   - Inject profiles into child models using parent's profile list
   - Ensure proper precedence where child elements override parent elements
   - Maintain correct inheritance hierarchy during profile activation

## Technical Details:

- Made DefaultProfileActivationContext.Record fields package-private for merging
- Added replayRecordIntoContext() method for proper key propagation
- Updated profile injection to respect inheritance precedence rules
- Enhanced MavenModelMerger to handle profile content merging correctly

## Benefits:

- Eliminates race conditions in concurrent model building scenarios
- Improves cache effectiveness through cleaner, more precise cache keys
- Ensures correct profile activation context propagation through inheritance
- Maintains proper property precedence in complex inheritance hierarchies
- Reduces memory usage by avoiding over-recording of intermediate keys

This fix ensures that profile activation behaves consistently and correctly
in both single-threaded and multi-threaded environments, while maintaining
optimal performance through improved caching mechanisms.
@gnodet gnodet requested a review from cstamas June 3, 2025 18:56
@gnodet gnodet added this to the 4.0.0-rc-4 milestone Jun 3, 2025
@gnodet
Copy link
Contributor Author

gnodet commented Jun 4, 2025

It also fails during execution:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce (default) on project flink-loader-query-history-export-job: Execution default of goal org.apache.maven.plugins:maven-enforcer-plugin:3.5.0:enforce failed: Failed to find reference POM which defines the current value -> [Help 1]

This looks like a different issue. Can you confirm this PR fixes the wrong computation of effective models ?

@gnodet gnodet added the bug Something isn't working label Jun 4, 2025
@wendigo
Copy link

wendigo commented Jun 4, 2025

@gnodet it does. I'm going to report a separate issue for the execution.

@gnodet gnodet merged commit 90457e5 into apache:master Jun 6, 2025
1 check passed
@jira-importer
Copy link

Resolve #9563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants