-
Notifications
You must be signed in to change notification settings - Fork 37
feat(jep379): Implement Shenandoah GC demo and update JDK15 metadata #312
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
base: master
Are you sure you want to change the base?
feat(jep379): Implement Shenandoah GC demo and update JDK15 metadata #312
Conversation
AloisSeckar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vanshslathia and thank you for your contribution. I have couple of remarks to be addressed before I can merge this PR. Please, have a look.
| java15DemoPool.add(new DatagramSocketDemo15()); | ||
| // JEP 375 | ||
| java15DemoPool.add(new InstanceofPatternMatchingSecondPreview()); | ||
| // JEP 379: Shenandoah: A Low-Pause-Time Garbage Collector (Production) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only place the JEP number here, as for the other demos
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here use the Markdown syntax comments, not old JavaDoc, as in all other demo implementations.
Check for the IDemo.java interface, there are instructions how it should look like.
| public void demo() { | ||
| info(379); | ||
|
|
||
| System.out.println("Shenandoah GC is primarily configured with JVM flags, not code."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not use println statements for explanation. Only use it, if it shows something directly related to the executed code. Turn extra information like this (rows 45-50) to classic comments inside the source code.
|
Since #313 was merged into main, which changes the way of loading the demo files, merge conflicts inevitably appeared in this PR and will have to be resolved. Please, sync your checkouts with current state of |
|
@vanshslathia will you be able to address the feedback and solve the merge conflicts? I tried to do it, but GitHub web editor cannot handle it, must be done locally in your checkout. Or you can create a new fork and re-create changes related to JEP 379. |
closes #295