Skip to content

Provides RSS annotations #39

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

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Jan 8, 2020

Fixes #21

This PR adds RSS annotations to measure the Resident Set Size of the process of the test methods.
This only works for Linux.

It adds the capability to gather process statistics so other annotations can take advantage of it.
It adds the capability from an anotation to skip the fork if needed.

@jeanbisutti
Copy link
Collaborator

Thanks for your work @loicmathieu!

I created a PR containing small minor improvements and the RSS tests ignored for Mac OS.

Feedbacks for improvement:

  • A test fails if the RSS exceeds this given by @ExpectRSS annotation.
    So, @ExpectRSS could be renamed into @ExpectMaxRSS.

  • Main issue
    A test annotated with a JVM annotation is launched in a specific JVM with the help of a ProcessBuilder (see NewJvmTestLauncher class). So the OS should create a specific process to run this test. Therefore, RSS would be measured without having to disable JVM forking in the following use cases mentioned by one of your code comment:

   * Some use case con take advantage of not forking to globally measure the RSS of the Test class,
     * taken into account the following part of the test execution:
     * - Global initilizer (static block, @BeforeAll or @BeforeClass annotated methods)
     * - Other extension that "starts an application" for integration testing (Spring or Quarkus test support)
     */
    boolean forkJvm() default true;
  • Could you please fix a compilation warning if it is needed to can skip a JVM fork from an annotation parameter:
[WARNING] ... SetOfAnnotationConfigs.java uses unchecked or unsafe operations.

RSS annotations are very promising! Thanks!

@loicmathieu
Copy link
Contributor Author

loicmathieu commented Jan 9, 2020

@jeanbisutti I merged your PR thanks.

RSS tests ignored for Mac OS.

MacOS is a POSIX system so it should works. We can merged as is and open an issue for it if I didn't find why it didn't works. Do you have a Mac? If so you can have a look maybe the directory for the PID file is not the same on MacOS and Linux ...

  • A test fails if the RSS exceeds this given by @ExpectRSS annotation.
    So, @ExpectRSS could be renamed into @ExpectMaxRSS.

Done

A test annotated with a JVM annotation is launched in a specific JVM with the help of a ProcessBuilder (see NewJvmTestLauncher class). So the OS should create a specific process to run this test. Therefore, RSS would be measured without having to disable JVM forking in the following use cases mentioned by one of your code comment [...]

I'm not happy with the fork/nofork dance and I need to check some usecases before deciding what to do with it. Maybe what we need is a way to define from wich PID we gather the statistics (default to the current one) as some framework could decide to start the process before (like Quarkus did for native tests). That's why it's still a draft PR :). I will take your feedback into account and analyse some usecase before going back with a better implementation.

Could you please create a PR for the documentation?

Yes, and I also need to make the documentation for the JUnit5 support :)

@jeanbisutti
Copy link
Collaborator

MacOS is a POSIX system so it should works. We can merged as is and open an issue for it if I didn't find why it didn't works. Do you have a Mac? If so you can have a look maybe the directory for the PID file is not the same on MacOS and Linux ...

I don't have a Mac... We could open an issue for this.

It seems a good idea to explore several use cases before choosing the final implementation.

@loicmathieu
Copy link
Contributor Author

Maybe what we need is a way to define from wich PID we gather the statistics (default to the current one)

I think the correct implementation for gathering the PID is default to the current one or the forked one in case of JVM forks. The RSS annotation should not mandate a fork by itself. Or should it ?

@jeanbisutti
Copy link
Collaborator

JVM annotation is launched in a specific JVM with the help of a ProcessBuilder (see NewJvmTestLauncher class). So the OS should create a specific process to run this test.

Therefore, I see several advantages of forking the JVM with the RSS annotations:

  • The stopRecording method of ProcessStatusRecorder could retrieve the current PID, after retrieve the RSS related to this PID and after save it in disk. The findRecord method of ProcessStatusRecorder would retrieve RSS value from disk. The implementation is rather simple.
  • Several tests measuring RSS and excuting concurrently would be independant

@loicmathieu
Copy link
Contributor Author

Several tests measuring RSS and excuting concurrently would be independant

Wether or not they measure RSS, several test executing concurrently or the one after the other could impact RSS significantly so yes, forking is needed ...

Anyway I want to test this with SringBoot/Micronaut/Quarkus integration tests as this annotation will make sense to measure RSS of an integration test not much of a simple unit test

@loicmathieu loicmathieu marked this pull request as ready for review February 14, 2020 14:44
@jeanbisutti jeanbisutti merged commit a495bb4 into quick-perf:master Feb 14, 2020
@jeanbisutti
Copy link
Collaborator

Thanks for your work @loicmathieu! It's great!

@loicmathieu
Copy link
Contributor Author

@jeanbisutti I remove the ability to avoid forking the VM as discussed.
This PR is ready now :)

@loicmathieu
Copy link
Contributor Author

Ah, just saw that it's already merged ;)

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.

Add @MeasureRSS and @ExpectRSS annotations
2 participants