RFR: 8203929: Limit amount of data for JFR.dump

[Markus Gronlund]

Attached another small section, most of them nits, feel free to incorporate or disregard as you see fit.

Thanks
Markus


-----Original Message-----
From: Markus Gronlund 
Sent: den 23 juni 2018 12:14
To: Erik Gahlin <erik.gahlin at oracle.com>; hotspot-jfr-dev at openjdk.java.net; Andrew Gross <andrew.gross at oracle.com>
Subject: RE: RFR: 8203929: Limit amount of data for JFR.dump

Hi Erik,

I am reading through the code and making some annotations. I have attached a description of my first pass of things, thought I send it your way so you have something to start with, more to come.

Do note the informal character of the review file, some sections are mainly clarifications for my own understanding, but I think you can infer from the context what are questions and suggestions.

Thanks
Markus

-----Original Message-----
From: Erik Gahlin 
Sent: den 21 juni 2018 18:16
To: hotspot-jfr-dev at openjdk.java.net; Andrew Gross <andrew.gross at oracle.com>
Subject: RFR: 8203929: Limit amount of data for JFR.dump

Hi,

Could I have a a review of an enhancement that will simplify the usage of JFR.dump and make it possible to copy parts of a recording to a file.  For more information, see the CSR:

https://bugs.openjdk.java.net/browse/JDK-8203930

Bug:
https://bugs.openjdk.java.net/browse/JDK-8203929

Webrev:
http://cr.openjdk.java.net/~egahlin/8203929/

Testing:
Tests in test/jdk/jdk/jfr

Thanks
Erik
-------------- next part --------------
// Part2

AbstractDcmd.java

 74             return makeGenerated(recording,path); <<-- space between arguments; -> (recording, path);

DcmdStart.java


if (Files.isDirectory(p) && Boolean.TRUE.equals(dumpOnExit)) {
 163                     // Decide destination filename at dump time
 164                     // Purposely avoided to generate filename in Recording#setDestination due to <<-- "Purposely avoided" -> "Purposely avoid "

 165                     // security concerns
 166                     PrivateAccess.getInstance().getPlatformRecording(recording).setDumpOnExitDirectory(new SafePath(p));
 167                 } else {
 168                     safePath = resolvePath(recording, path);
 169                     recording.setDestination(safePath.toPath());
 170                 }



OldObjectSample.java

  35
  36     // Emit if old object is enabled for at lest one recording, and use the largest <<-- "lest" --> "at least"


jfrDcmds.cpp

jfrDumpFlightRecording:

  _filename("filename", "Copy recording data to file, i.e \\\"" JFR_FILENAME_EXAMPLE "\\\"", "STRING", false), <<-- i.e -> e.g. as it is an example (consistent with the other parameters) (not your change but maybe fix anyway?)


 _dcmdparser.add_dcmd_option(&_filename);
 191   _dcmdparser.add_dcmd_option(&_begin); <<-- add them in order as listed in the constructor ?
 192   _dcmdparser.add_dcmd_option(&_end);
 193   _dcmdparser.add_dcmd_option(&_maxage);
 194   _dcmdparser.add_dcmd_option(&_maxsize);
 195   _dcmdparser.add_dcmd_option(&_path_to_gc_roots);


PlatformRecorder.java

In general, can we move the logging outside of the recorder lock?


 149         RecordingState oldState;
 150         RecordingState newState;
 151         synchronized (recorder) {
 152             oldState = getState();
 153             if (stopTask != null) {
 154                 stopTask.cancel();
 155                 stopTask = null;
 156             }
 157             recorder.stop(this);
 158             String endText = reason == null ? "" : ". Reason \"" + reason + "\".";
 159             Logger.log(LogTag.JFR, LogLevel.INFO, "Stopped recording \"" + getName() + "\" (" + getId() + ")" + endText); <<-- maybe we can log outside of the recorder lock?
 160             this.stopTime = Instant.now();
 161             newState = getState();
 162         }
 163         WriteableUserPath dest = getDestination();


public void scheduleStart(Duration delay) {
 181         synchronized (recorder) {
 182             ensureOkForSchedule();
 183
 184             startTime = Instant.now().plus(delay);
 185             LocalDateTime now = LocalDateTime.now().plus(delay);
 186             setState(RecordingState.DELAYED);
 187             startTask = createStartTask();
 188             recorder.getTimer().schedule(startTask, delay.toMillis());
 189             Logger.log(LogTag.JFR, LogLevel.INFO, "Scheduled recording \"" + getName() + "\" (" + getId() + ") to start at " + now); <<-- same here?
 190         }
 191     }
 192


TestJcmdDumpGeneratedFilename.java

  52         try (Recording r = new Recording(Configuration.getConfiguration("default"))) {
  53             r.start();
  54             r.stop(); <----------------- should the recording really be stopped before attempting dump? A more common use case is probably to dump a running recording? i.e. do r.stop() last?
  55             testDumpFilename();
  56             testDumpFilename(r);
  57             testDumpDiectory(); <<-- testDumpDiectory() -> testDumpDirectory()
  58             testDumpDiectory(r);
  59         }
  60     }

TestJcmdDumpLimited.java

 69         TestRecording(int id, int events) throws IOException, InterruptedException { <<-- maybe the paramater should change from "events" to "eventCount"?

  81             r.stop();  <<----------------------------------
  82             Thread.sleep(1);
  83             path = Paths.get("dump-" + id + ".jfr");
  84             r.dump(path); <<---------------------------------- this test also dumps an already stopped recording. Is this a requirement?
  85             size = (int) Files.size(path);
  86             this.id = id;
  87             this.now = Instant.now();