Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JVM-2571: BlackholeSyncEATest is broken after PEA is enabled #89

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

navyxliu
Copy link
Owner

This patch fixed regression test 'BlackholeSyncTest' on all platforms. It explicitly disables PEA in the testcase because it contradicts the assumption.

PEA postpones the object allocation to its escaping. If the transformation surpass a synchronized(o) {} construct, it's possible to elide the lock. We explicitly disable PEA in the testcase so it only tests C2 Escape Analysis.

@caojoshua
Copy link
Collaborator

I'm surprised EA cannot ellide the lock in the blackhole case. I guess control flow analysis through PEA helps

@navyxliu
Copy link
Owner Author

@caojoshua We did. The test expects to capture FastLock/FastUnlock nodes but PEA manage to elide them. that's why the test failed.

Aleksei says that the semantic of 'blackhole(x)' to make o 'GlobalEscaped'.
PEA take actions even before parser encounter Blackhole node. in current design, it's hard to respect the semantic of blackhole.

    @Test
    @IR(counts = {IRNode.FAST_LOCK, "1"})
    @IR(counts = {IRNode.FAST_UNLOCK, "1"})
    static void testBlackholed() {
        Object o = new Object();
        synchronized (o) {} // elided by PEA
        blackhole(o);
    }

@navyxliu
Copy link
Owner Author

Let's ignore blackhole node for time being. we will revisit this issue with a new design.

@navyxliu navyxliu closed this Jan 17, 2024
@navyxliu navyxliu reopened this Jan 18, 2024
@navyxliu navyxliu merged commit 239b455 into PEA_beta Jan 18, 2024
89 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants