RFR: 8308252: Refactor line-by-line file reading code [v3]
Thomas Stuefe
stuefe at openjdk.org
Thu May 18 09:54:58 UTC 2023
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
What would cool would be a closure that gets each line, optionally with the ability to stop iteration prematurely. Or even better, a lambda that does the same.
src/hotspot/share/cds/classListParser.cpp line 63:
> 61: if (!_reader.is_opened()) {
> 62: char errmsg[JVM_MAXPATHLEN];
> 63: os::lasterror(errmsg, JVM_MAXPATHLEN);
_reader should buffer errno after the failing OS call. We should not have to rely on os::lasterror() being called right after whatever OS API failed inside the reader. Neither is os::lasterror() necessary, we can just use os::strerror since reader only uses Posix file APIs.
src/hotspot/share/utilities/lineReader.cpp line 30:
> 28: #include "utilities/lineReader.hpp"
> 29:
> 30: LineReader::LineReader(const char* filename) : _filename(filename), _stream(nullptr) {
Maybe strdup the file name to be sure? Up to you. We usually just feed literals, so this may be ok.
src/hotspot/share/utilities/lineReader.cpp line 44:
> 42: }
> 43: } else {
> 44: _stream = nullptr;
unnecessary
src/hotspot/share/utilities/lineReader.cpp line 65:
> 63: // \n is treated as the line separator.
> 64: // All occurrences of \r are stripped.
> 65: char* LineReader::get_line() {
I would return const char* here, this is an internal buffer.
src/hotspot/share/utilities/lineReader.cpp line 71:
> 69: size_t buffer_pos = 0;
> 70: int c;
> 71: while ((c = getc(_stream)) != EOF) {
Lets not read individual characters. Lets use fgets or fread() or just plain read(). Preferably the first.
src/hotspot/share/utilities/lineReader.cpp line 76:
> 74: if (new_length < _buffer_length) {
> 75: // This could happen on 32-bit. On 64-bit, the VM would have exited
> 76: // due to OOM before we ever get to here.
This is scary. I don't like a general utility to use half my address space on 32-bit if bad things happen. I would cap the max. buffer size to something sensible, e.g. 1K or 64K.
-------------
Changes requested by stuefe (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14025#pullrequestreview-1432366585
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197607457
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197616845
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197617824
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197626049
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197623702
PR Review Comment: https://git.openjdk.org/jdk/pull/14025#discussion_r1197624433
More information about the hotspot-dev
mailing list