Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve OTB loader iteration #4795

Draft
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

ranisalt
Copy link
Member

@ranisalt ranisalt commented Sep 25, 2024

Pull Request Prelude

Changes Proposed

Improves file loader performance by having less indirection and skipping checks aggressively. Loading the map is at least 20% faster (4s -> 3.2s) on my machine using EPuncker/orts2 map.

When running a release build, most of the sanity checks will be skipped (e.g. checking if the file does not underflow, if the nodes have expected types) since we expect map editors to generate correct map files. The map only needs to be loaded once with a debug build if one wants to make sure it works, so there is no need to check again on every start.

Copy link
Contributor

@ramon-bernardo ramon-bernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rani, here are some considerations I noted while reviewing;

  • do we have something like a debug assert? Is it the same thing?

src/item.h Outdated Show resolved Hide resolved
src/iologindata.cpp Show resolved Hide resolved
src/podium.cpp Outdated
}

uint8_t flags = 0;
propStream.read<uint8_t>(flags);
uint8_t flags = OTB::read<uint8_t>(first, last);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use auto in most cases here.

src/bed.cpp Outdated Show resolved Hide resolved
src/items.cpp Outdated Show resolved Hide resolved
src/iomap.h Outdated Show resolved Hide resolved
src/iomap.cpp Outdated Show resolved Hide resolved
src/iomap.cpp Outdated Show resolved Hide resolved
src/fileloader.h Show resolved Hide resolved
src/fileloader.cpp Outdated Show resolved Hide resolved
@ranisalt
Copy link
Member Author

Just learned about [[likely]]/[[unlikely]] in C++20 that allows me to keep ifs instead of asserts with virtually no penalty, but with much better error handling (catchable exceptions with meaningful messages)

src/iomap.cpp Outdated Show resolved Hide resolved
src/iomap.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@ramon-bernardo ramon-bernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more points to consider; I really liked the new approach!

Unfortunately, I won't be able to test these changes (VS is having some issues...)

@maattch maattch mentioned this pull request Oct 7, 2024
3 tasks
@ranisalt
Copy link
Member Author

ranisalt commented Oct 8, 2024

I'm still working on it to make a single pass loader, it should save a significant amount of time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants