Skip to content

Provides RSS annotations#39

Merged
jeanbisutti merged 1 commit into
quick-perf:masterfrom
loicmathieu:feat/rss-2
Feb 14, 2020
Merged

Provides RSS annotations#39
jeanbisutti merged 1 commit into
quick-perf:masterfrom
loicmathieu:feat/rss-2

Conversation

@loicmathieu

@loicmathieu loicmathieu commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

loicmathieu commented Jan 9, 2020

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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

@loicmathieu

Copy link
Copy Markdown
Contributor Author

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

@loicmathieu

Copy link
Copy Markdown
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