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

Use GetExtensionEvents to derive extension event numbers #545

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

babsingh
Copy link
Member

@babsingh babsingh commented Feb 1, 2023

Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh [email protected]

@keithc-ca
Copy link
Member

Why is this necessary at all? We go to some effort in jvmti.h.m4 to (apparently) match the RI event numbers. Perhaps that's where the problem lies?

@babsingh
Copy link
Member Author

babsingh commented Feb 2, 2023

Why is this necessary at all? We go to some effort in jvmti.h.m4 to (apparently) match the RI event numbers. Perhaps that's where the problem lies?

@gacholio mentioned that the current behaviour is invalid and not supported by the JVMTI specification. More details can be found in the comments surrounding eclipse-openj9/openj9#16501 (comment).

@keithc-ca
Copy link
Member

@gacholio mentioned that the current behaviour is invalid and not supported by the JVMTI specification. More details can be found in the comments surrounding eclipse-openj9/openj9#16501 (comment).

I'm not saying it isn't questionable practice to hard-code those constants, but I expect they will be constants, and others will do likewise, so we're only hurting ourselves by playing hardball here. Hopefully they'll be defined in jvmti.h for users to see.

@gacholio
Copy link
Member

gacholio commented Feb 2, 2023

Extension events are by definition not in the header file since they are not expected to be the same between VM implementations. There is a specified way to get the numbers from the data returned from the extension query.

@keithc-ca
Copy link
Member

Ok got it. Thanks.

@keithc-ca
Copy link
Member

Once we settle on a solution here, we should be proposing the fix upstream.

@babsingh babsingh force-pushed the fix_exteventnumbers branch from de6aa95 to db6f8df Compare February 2, 2023 20:09
@babsingh
Copy link
Member Author

babsingh commented Feb 2, 2023

we should be proposing the fix upstream.

by upstream, do we mean https://github.com/openjdk/jdk?

@babsingh babsingh force-pushed the fix_exteventnumbers branch from db6f8df to dda36cd Compare February 2, 2023 20:48
@gacholio
Copy link
Member

gacholio commented Feb 3, 2023

As this is throw-away code, the most likely way to get changes accepted upstream would be to make the smallest change possible, not the most elegant.

@keithc-ca
Copy link
Member

by upstream, do we mean https://github.com/openjdk/jdk?

Yes.

@keithc-ca
Copy link
Member

I think this should wait for the resolution of #531 (comment).

Currently, extension event numbers are hard-coded in the tests. This is
invalid since the extension event numbers can vary between
implementations. Instead, GetExtensionEvents should be used to derive
the extension event numbers.

Related: eclipse-openj9/openj9#16501
Related: eclipse-openj9/openj9#16167

Signed-off-by: Babneet Singh <[email protected]>
@babsingh babsingh force-pushed the fix_exteventnumbers branch from dda36cd to d3f3782 Compare February 7, 2023 04:04
@babsingh
Copy link
Member Author

babsingh commented Feb 7, 2023

I think this should wait for the resolution of #531 (comment).

None of the files in this PR are updated in #531 (comment). There is no noticeable dependency. I don't see a reason to wait.

@keithc-ca
Copy link
Member

I don't see a reason to wait.

The changes here appear to be the same as in ibmruntimes/openj9-openjdk-jdk20#13; once I'm satisfied with the latter, I think it should be safe to merge this.

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.

3 participants