On Wed, 7 Apr 2021 22:44:41 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Hi @sspitsyn, >> >> I rather like the much simpler agent code and it puts the majority of logic >> in the .java file so the reader/reviewer doesn't have to jump back and forth >> between the two files nearly as much. >> >> The `id` argument had to be passed to the old version of the function calls >> in order for the logging messages generated by the agent code to (easily) >> have the same thread names as the Java side logging. Once all the error >> checking was moved to the Java side, that made the `id` argument no longer >> necessary and also allowed the native `check_jvmti_status()` function (along >> with other functions) to be removed. So there was nothing wrong with the >> native `check_jvmti_status()` function; it was just made obsolete by the code >> migration to the Java side. >> >> I consider @robehn's idea here to be good evolution for these kinds of tests >> to >> move code from the native side, where the code is more platform dependent, >> to the Java side, where the code is more platform independent. >> >> I hope I have convinced you that this is a good changeset for moving forward. > > Dan, > I'm sorry, I do not see much value in this approach going forward. One line > with a call to check_jvmti_status() does not add much to the platform > dependency no to complexity in native code. Also, more sophisticated analysis > of the returned error code frequently is needed. Also, I consider it not > feasible to move a thousand of JVMTI tests in this direction. So that we will > end up with inconsistency over all tests. > I've already marked this as reviewed, so you can push it. But I'm against > following this pattern going forward. Or at least, we need to consult with > the SQE engineers first. > Thanks, > Serguei @sspitsyn - First, thanks for approving the changes! Second, I'm _not_ proposing changing any other JVM/TI tests to use this style. The only reason that this style works (for me and @robehn) is because none of these JVM/TI calls is expected to fail. The tests are carefully constructed to exercise the code for a race condition that will result in a hang or a crash (if something goes wrong). In the case of these tests, we don't need sophisticated analysis of the returned error codes. Again, thanks for approving these tests. ------------- PR: https://git.openjdk.java.net/jdk/pull/2899
