Skip to content

Commit

Permalink
VariantParser improvements to readahead
Browse files Browse the repository at this point in the history 
Reintroduces readahead to ResourceInteractiveLoaderText, except in cases where file pointers are modified externally.
Adds a new safe mechanism to open FileAccess within VariantParser::StreamFile, rather than relying on externally supplied FileAccess.
  • Loading branch information
@lawnjelly
lawnjelly committed Dec 18, 2022
1 parent 6963ba6 commit 24f7125
Show file tree
Hide file tree
Showing 10 changed files with 278 additions and 95 deletions.
2 changes: 1 addition & 1 deletion core/io/config_file.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ Error ConfigFile::load_encrypted_pass(const String &p_path, const String &p_pass

Error ConfigFile::_internal_load(const String &p_path, FileAccess *f) {
VariantParser::StreamFile stream;
stream.f = f;
stream.set_file(f, VariantParser::Stream::READAHEAD_ENABLED);

Error err = _parse(p_path, &stream);

Expand Down
27 changes: 6 additions & 21 deletions core/io/resource_importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,15 @@ bool ResourceFormatImporter::SortImporterByName::operator()(const Ref<ResourceIm
}

Error ResourceFormatImporter::_get_path_and_type(const String &p_path, PathAndType &r_path_and_type, bool *r_valid) const {
Error err;
FileAccess *f = FileAccess::open(p_path + ".import", FileAccess::READ, &err);

if (!f) {
VariantParser::StreamFile stream;
Error err = stream.open_file(p_path + ".import");
if (err != OK) {
if (r_valid) {
*r_valid = false;
}
return err;
}

VariantParser::StreamFile stream;
stream.f = f;

String assign;
Variant value;
VariantParser::Tag next_tag;
Expand All @@ -70,11 +66,9 @@ Error ResourceFormatImporter::_get_path_and_type(const String &p_path, PathAndTy

err = VariantParser::parse_tag_assign_eof(&stream, lines, error_text, next_tag, assign, value, nullptr, true);
if (err == ERR_FILE_EOF) {
memdelete(f);
return OK;
} else if (err != OK) {
ERR_PRINT("ResourceFormatImporter::load - " + p_path + ".import:" + itos(lines) + " error: " + error_text);
memdelete(f);
return err;
}

Expand Down Expand Up @@ -108,8 +102,6 @@ Error ResourceFormatImporter::_get_path_and_type(const String &p_path, PathAndTy
}
}

memdelete(f);

if (r_path_and_type.path == String() || r_path_and_type.type == String()) {
return ERR_FILE_CORRUPT;
}
Expand Down Expand Up @@ -243,16 +235,12 @@ String ResourceFormatImporter::get_internal_resource_path(const String &p_path)
}

void ResourceFormatImporter::get_internal_resource_path_list(const String &p_path, List<String> *r_paths) {
Error err;
FileAccess *f = FileAccess::open(p_path + ".import", FileAccess::READ, &err);

if (!f) {
VariantParser::StreamFile stream;
Error err = stream.open_file(p_path + ".import");
if (err != OK) {
return;
}

VariantParser::StreamFile stream;
stream.f = f;

String assign;
Variant value;
VariantParser::Tag next_tag;
Expand All @@ -266,11 +254,9 @@ void ResourceFormatImporter::get_internal_resource_path_list(const String &p_pat

err = VariantParser::parse_tag_assign_eof(&stream, lines, error_text, next_tag, assign, value, nullptr, true);
if (err == ERR_FILE_EOF) {
memdelete(f);
return;
} else if (err != OK) {
ERR_PRINT("ResourceFormatImporter::get_internal_resource_path_list - " + p_path + ".import:" + itos(lines) + " error: " + error_text);
memdelete(f);
return;
}

Expand All @@ -284,7 +270,6 @@ void ResourceFormatImporter::get_internal_resource_path_list(const String &p_pat
break;
}
}
memdelete(f);
}

String ResourceFormatImporter::get_import_group_file(const String &p_path) const {
Expand Down
12 changes: 3 additions & 9 deletions core/io/resource_loader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -768,13 +768,9 @@ String ResourceLoader::_path_remap(const String &p_path, bool *r_translation_rem
new_path = path_remaps[new_path];
} else {
// Try file remap.
Error err;
FileAccess *f = FileAccess::open(new_path + ".remap", FileAccess::READ, &err);

if (f) {
VariantParser::StreamFile stream;
stream.f = f;

VariantParser::StreamFile stream;
Error err = stream.open_file(new_path + ".remap");
if (err == OK) {
String assign;
Variant value;
VariantParser::Tag next_tag;
Expand All @@ -801,8 +797,6 @@ String ResourceLoader::_path_remap(const String &p_path, bool *r_translation_rem
break;
}
}

memdelete(f);
}
}

Expand Down
13 changes: 3 additions & 10 deletions core/project_settings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,18 +581,14 @@ Error ProjectSettings::_load_settings_binary(const String &p_path) {
}

Error ProjectSettings::_load_settings_text(const String &p_path) {
Error err;
FileAccess *f = FileAccess::open(p_path, FileAccess::READ, &err);

if (!f) {
VariantParser::StreamFile stream;
Error err = stream.open_file(p_path);
if (err != OK) {
// FIXME: Above 'err' error code is ERR_FILE_CANT_OPEN if the file is missing
// This needs to be streamlined if we want decent error reporting
return ERR_FILE_NOT_FOUND;
}

VariantParser::StreamFile stream;
stream.f = f;

String assign;
Variant value;
VariantParser::Tag next_tag;
Expand All @@ -609,23 +605,20 @@ Error ProjectSettings::_load_settings_text(const String &p_path) {

err = VariantParser::parse_tag_assign_eof(&stream, lines, error_text, next_tag, assign, value, nullptr, true);
if (err == ERR_FILE_EOF) {
memdelete(f);
// If we're loading a project.godot from source code, we can operate some
// ProjectSettings conversions if need be.
_convert_to_last_version(config_version);
last_save_time = FileAccess::get_modified_time(get_resource_path().plus_file("project.godot"));
return OK;
} else if (err != OK) {
ERR_PRINT("Error parsing " + p_path + " at line " + itos(lines) + ": " + error_text + " File might be corrupted.");
memdelete(f);
return err;
}

if (assign != String()) {
if (section == String() && assign == "config_version") {
config_version = value;
if (config_version > CONFIG_VERSION) {
memdelete(f);
ERR_FAIL_V_MSG(ERR_FILE_CANT_OPEN, vformat("Can't open project at '%s', its `config_version` (%d) is from a more recent and incompatible version of the engine. Expected config version: %d.", p_path, config_version, CONFIG_VERSION));
}
} else {
Expand Down
161 changes: 146 additions & 15 deletions core/variant_parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,38 +35,156 @@
#include "core/os/keyboard.h"
#include "core/string_buffer.h"

// Keep this function non-virtual, and as optimized as possible for the common case.
CharType VariantParser::Stream::get_char() {
// is within buffer?
if (readahead_pointer < readahead_filled) {
return readahead_buffer[readahead_pointer++];
}

// attempt to readahead
readahead_filled = _read_buffer(readahead_buffer, readahead_enabled ? READAHEAD_SIZE : 1);
readahead_filled = _read_buffer(readahead_buffer, readahead_size);
if (readahead_filled) {
readahead_pointer = 0;
} else {
// EOF
readahead_pointer = 1;
eof = true;
_eof = true;
return 0;
}
return get_char();
}

bool VariantParser::Stream::is_eof() const {
if (readahead_enabled) {
return eof;
return _eof;
}

uint32_t VariantParser::Stream::get_readahead_offset() const {
DEV_ASSERT(!_eof || ((readahead_filled - readahead_pointer) == 0));
// Uncomment the next line if the assert is being hit.
// if (_eof) {return 0;}
return readahead_filled - readahead_pointer;
}

void VariantParser::Stream::invalidate_readahead(bool p_eof) {
readahead_filled = 0;
readahead_pointer = p_eof ? 1 : 0;
_eof = p_eof;
}

Error VariantParser::StreamFile::open_file(const String &p_path) {
Error err;
FileAccess *f = FileAccess::open(p_path, FileAccess::READ, &err);

if (!f) {
// double check we are not sending an OK in case of failure.
if (err == OK) {
err = FAILED;
}
return err;
}

if (err == OK) {
// Always use readahead for owned files.
// This is always safe, because the file pointer etc cannot be changed
// externally, and the chief reason for having owned files.
set_file(f, READAHEAD_ENABLED);
_is_owned_file = true;
}

return err;
}

Error VariantParser::StreamFile::close_file() {
if (_is_owned_file) {
if (_file) {
memdelete(_file);
_file = nullptr;
_is_owned_file = false;
invalidate_readahead();
return OK;
}
// This should probably never occur, but just in case...
_is_owned_file = false;
}

return FAILED;
}

VariantParser::StreamFile::~StreamFile() {
// free any owned file
close_file();
}

void VariantParser::StreamFile::set_file(FileAccess *p_file, Readahead p_readahead) {
// free any existing owned file
close_file();

bool use_readahead = p_readahead == READAHEAD_ENABLED;

// noop?
if ((p_file == _file) && (use_readahead == readahead_enabled())) {
return;
}

_file = p_file;

// set_file defaults to a non-owned file,
// if we are setting an owned file we must set this bool
// immediately after set_file().
_is_owned_file = false;

readahead_size = use_readahead ? READAHEAD_SIZE : 1;
invalidate_readahead();

// Files should be opened BEFORE calling set_file.
ERR_FAIL_COND(_file && !_file->is_open());
}

void VariantParser::StreamFile::invalidate_readahead() {
bool eof = true;
if (_file) {
bool open = _file->is_open();
eof = open ? _file->eof_reached() : false;

VariantParser::Stream::invalidate_readahead(eof);
#ifdef DEV_ENABLED
_readahead_start_source_pos = open ? _file->get_position() : 0;
_readahead_end_source_pos = _readahead_start_source_pos;
#endif
} else {
VariantParser::Stream::invalidate_readahead(true);
#ifdef DEV_ENABLED
_readahead_start_source_pos = 0;
_readahead_end_source_pos = 0;
#endif
}
return _is_eof();
}

uint32_t VariantParser::StreamFile::_read_buffer(CharType *p_buffer, uint32_t p_num_chars) {
// The buffer is assumed to include at least one character (for null terminator)
ERR_FAIL_COND_V(!p_num_chars, 0);
DEV_ASSERT(p_num_chars);
ERR_FAIL_NULL_V_MSG(_file, 0, "Attempting to read from NULL file.");

#ifdef DEV_ENABLED
if (readahead_enabled()) {
_readahead_start_source_pos = _file->get_position();
if (_readahead_start_source_pos != _readahead_end_source_pos) {
ERR_PRINT_ONCE("StreamFile out of sync. HIGH PRIORITY bug, please report on github.");
return 0;
}
}
#endif

uint8_t *temp = (uint8_t *)alloca(p_num_chars);
uint64_t num_read = f->get_buffer(temp, p_num_chars);
uint64_t num_read = _file->get_buffer(temp, p_num_chars);

#ifdef DEV_ENABLED
if (readahead_enabled()) {
_readahead_end_source_pos = _file->get_position();
}
#endif

ERR_FAIL_COND_V(num_read == UINT64_MAX, 0);

// translate to wchar
Expand All @@ -83,29 +201,42 @@ bool VariantParser::StreamFile::is_utf8() const {
}

bool VariantParser::StreamFile::_is_eof() const {
return f->eof_reached();
ERR_FAIL_NULL_V(_file, true);
return _file->eof_reached();
}

uint64_t VariantParser::StreamFile::get_position() const {
ERR_FAIL_NULL_V(_file, -1);

// Note this assumes that get_position() returns get_length() when EOF
// is reached. Watch for bugs here.
return _file->get_position() - get_readahead_offset();
}

uint64_t VariantParser::StreamString::get_position() const {
return _pos - get_readahead_offset();
}

uint32_t VariantParser::StreamString::_read_buffer(CharType *p_buffer, uint32_t p_num_chars) {
// The buffer is assumed to include at least one character (for null terminator)
ERR_FAIL_COND_V(!p_num_chars, 0);
DEV_ASSERT(p_num_chars);

int available = MAX(s.length() - pos, 0);
int available = MAX(s.length() - _pos, 0);
if (available >= (int)p_num_chars) {
const CharType *src = s.ptr();
src += pos;
src += _pos;
memcpy(p_buffer, src, p_num_chars * sizeof(CharType));
pos += p_num_chars;
_pos += p_num_chars;

return p_num_chars;
}

// going to reach EOF
if (available) {
const CharType *src = s.ptr();
src += pos;
src += _pos;
memcpy(p_buffer, src, available * sizeof(CharType));
pos += available;
_pos += available;
}

// add a zero
Expand All @@ -119,7 +250,7 @@ bool VariantParser::StreamString::is_utf8() const {
}

bool VariantParser::StreamString::_is_eof() const {
return pos > s.length();
return _pos > s.length();
}

/////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
Loading

0 comments on commit 24f7125

Please sign in to comment.