RFR: 8330532: Improve line-oriented text parsing in HotSpot

Coleen Phillimore coleenp at openjdk.org
Thu Apr 18 12:11:57 UTC 2024


On Thu, 18 Apr 2024 03:51:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> (This PR is an alternative to https://github.com/openjdk/jdk/pull/18669 with a better API for reading lines of text)
> 
> HotSpot has a few cases where information is parsed from a file, or from a memory buffer, one line at a time. Example:
> 
> - https://github.com/openjdk/jdk/blob/064628471b83616b4463baa78618d1b7a66d0c7c/src/hotspot/share/cds/classListParser.cpp#L169
> - https://github.com/openjdk/jdk/blob/064628471b83616b4463baa78618d1b7a66d0c7c/src/hotspot/share/compiler/compilerOracle.cpp#L1059-L1066
> 
> Common problems:
> - They use a fixed buffer for reading a line, so long (but valid) lines will cause errors.
> - There's ad-hoc code that deals with `FILE*` differently than from memory.
> 
> This RFE implements a common utility, `inputStream`, for reading lines from different sources of input (see `FileInput` and `MemoryInput`). We fixed only `ClassListParser` and `CompilerOracle` in this RFE, but we can fix other readers in follow-up RFEs.
> 
> The API allows other source of input to be implemented. For example, one could implement a `SocketInput` if there's a use case for it.
> 
> In the future, `inputStream` can be extended (or encapsulated in a higher-level reader class) to read typed input tokens (for example, integers, strings, etc.)
> 
> Credit:
> The `inputStream` class and friends are contributed by @rose00 . See https://mail.openjdk.org/pipermail/hotspot-dev/2024-April/087077.html .
> 
> John's original version is in the draft PR https://github.com/openjdk/jdk/pull/18773. In order to minimize the size of this PR, I have kept only the functionalities for reading a line and a time. Other features, such as pushing back contents into the `inputStream`, could be added in follow-up PRs. (These removed features can be found in the commit history of this PR).

Just drive-by comments.

src/hotspot/share/cds/classListParser.cpp line 436:

> 434: }
> 435: 
> 436: void ClassListParser::check_class_name(const char* class_name) {

Did we not already have code to check the length of the class name for the class list parser?  There's similar code in systemDictionary.

src/hotspot/share/utilities/istream.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.

These new files should only say 2024 in the copyright.

src/hotspot/share/utilities/istream.hpp line 234:

> 232:     _line_ending = 0;
> 233:     _input_state = NTR_STATE;
> 234:   }

This should be an initialization list.

-------------

PR Review: https://git.openjdk.org/jdk/pull/18833#pullrequestreview-2008679215
PR Review Comment: https://git.openjdk.org/jdk/pull/18833#discussion_r1570587600
PR Review Comment: https://git.openjdk.org/jdk/pull/18833#discussion_r1570581054
PR Review Comment: https://git.openjdk.org/jdk/pull/18833#discussion_r1570585826


More information about the hotspot-dev mailing list