On Mon, 2 Dec 2024 13:39:57 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which removes usages of >> SecurityManager related APIs and some leftover related to SecurityManager >> changes? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these >> changes are trivial. The >> `src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java` used to >> expose utility methods for dealing with SecurityManager permissions and it >> was called from a few places. That class is no longer needed with the clean >> up done in this PR. >> >> No new tests have been introduced and tier testing is currently in progress. > > Jaikiran Pai has updated the pull request incrementally with four additional > commits since the last revision: > > - remove unnecessary space > - Path.of() instead of Paths.get() > - fix formatting of try-with-resources in CgroupSubsystemController.java > - leave out MethodUtil from the clean up I've reviewed only the cgroups parts. src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java line 70: > 68: String line = bufferedReader.readLine(); > 69: return line; > 70: } catch (IOException e) { We can use `Files.lines` here right away: Suggestion: try (Stream<String> lines = Files.lines(filePath)) { Optional<String> firstLine = lines.findFirst(); return firstLine.orElse(null); } catch (UncheckedIOException | IOException e) { src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java line 71: > 69: String line = bufferedReader.readLine(); > 70: return line; > 71: } catch (IOException e) { I think we can do this refactoring now: Suggestion: try (Stream<String> lines = Files.lines(Paths.of(controller.path(), param))) { Optional<String> firstLine = lines.findFirst(); return firstLine.orElse(null); } catch (IOException e) { src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java line 332: > 330: > 331: private long sumTokensIOStat(Function<String, Long> mapFunc) { > 332: try (Stream<String> lines = > Files.lines(Paths.get(unified.path(), "io.stat"))) { Suggestion: try (Stream<String> lines = Files.lines(Paths.of(unified.path(), "io.stat"))) { src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java line 334: > 332: try (Stream<String> lines = > Files.lines(Paths.get(unified.path(), "io.stat"))) { > 333: return lines.map(mapFunc) > 334: .collect(Collectors.summingLong(e -> e)); Suggestion: return lines.map(mapFunc) .collect(Collectors.summingLong(e -> e)); ------------- PR Review: https://git.openjdk.org/jdk/pull/22478#pullrequestreview-2472643767 PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865876078 PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865860841 PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865815063 PR Review Comment: https://git.openjdk.org/jdk/pull/22478#discussion_r1865865085
