RFR: 8308252: Refactor line-by-line file reading code [v3]

[David Holmes]

On Wed, 17 May 2023 19:55:59 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> I extracted the `get_line()` code from `CompileReplay` and put it in a utility class so that it can be used by `ClassListParser` as well. A few notable changes:
>> 
>> - Simplified the API
>> - Changed the buffer size to a size_t
>> - Added size overflow and OOM checks
>> - Brought over the `fdopen` logic from `ClassListParser` for handling long path names on Windows. (I don't know how valid this is nowadays, but I don't want to drop it in a refactoring PR).
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed typo in comments

Looks useful. I wonder if the argument file processing  logic might benefit from this too?

A few comments below.

Thanks.

src/hotspot/share/utilities/lineReader.cpp line 32:

> 30: LineReader::LineReader(const char* filename) : _filename(filename), _stream(nullptr) {
> 31:   // Use os::open() because neither fopen() nor os::fopen()
> 32:   // can handle long path name on Windows. HMM, is this still valid today???

It is not clear to me that any of these API's avoid the MAX_PATH 260 character limitation as the only way around that is to use `\?` UNC path naming - which we don't appear to do anywhere. On Windows 10 and later if  the machine/user is configured for long paths by default then the `FindFirstFile` used by `os::open` may avoid the 260 limit, but only if it actually maps to the underlying unicode version of the method.
Anyway something to follow up on in a separate RFE I think.

src/hotspot/share/utilities/lineReader.cpp line 47:

> 45:   }
> 46: 
> 47:   _buffer_length = 32;

This default buffer size may work well for the `ciReplay` case but for a general utility there should probably be a way to set the initial buffer length to avoid unnecessary resizing.

src/hotspot/share/utilities/lineReader.cpp line 73:

> 71:   while ((c = getc(_stream)) != EOF) {
> 72:     if (buffer_pos + 1 >= _buffer_length) {
> 73:       size_t new_length = _buffer_length * 2;

Again for a general utility this simple doubling of size may not be appropriate. For small size it should be okay though. And if we can set the initial size to avoid the need to grow then this is less of an issue.

src/hotspot/share/utilities/lineReader.hpp line 43:

> 41:   ~LineReader();
> 42: 
> 43:   bool is_opened() const {

Nit: I'd probably go for `is_open()`

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

PR Review: https://git.openjdk.org/jdk/pull/14025#pullrequestreview-1431846473
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197207428
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197211575
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197213253
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197209637