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

Add a string_view variant of base::split_string() #117

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

ckaiser
Copy link
Collaborator

@ckaiser ckaiser commented Dec 5, 2024

Adds a string_view version of base::split_string

Benchmarked difference between the two versions of the function puts the string_view version at around 36-37% faster, although it wasn't extensive testing, just a little microbenchmark:

  auto t0 = std::chrono::high_resolution_clock::now();
  {
    const std::string_view string = "Hello,World";
    for (int i = 0; i < 200000; i++) {
      std::vector<std::string_view> result;
      base::split_string("Hello,World", result, ",");
    }
  }

  auto t1 = std::chrono::high_resolution_clock::now();
  auto view = std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();
  t0 = std::chrono::high_resolution_clock::now();

  {
    const std::string string2 = "Hello,World";
    for (int i = 0; i < 200000; i++) {
      std::vector<std::string> result;
      base::split_string(string2, result, ",");
    }
  }

  t1 = std::chrono::high_resolution_clock::now();
  auto noView = std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();

  std::cout << "string_view: " << view << " us" << std::endl;
  std::cout << "string:      " << noView << " us" << std::endl;

Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

base/split_string.cpp Show resolved Hide resolved
base/split_string_tests.cpp Show resolved Hide resolved
base/split_string_tests.cpp Show resolved Hide resolved
base/split_string_tests.cpp Show resolved Hide resolved
@dacap
Copy link
Member

dacap commented Dec 5, 2024

Hi @ckaiser, we were avoiding std::string_view at the moment but just for fear of macOS 10.9 incompatibilities. The GitHub CI worked (it compiles with CMAKE_OSX_DEPLOYMENT_TARGET=10.9 for macOS in the same way we compile the official distribution) so it should be safe to start using std::string_view (there are a lot of TODO/places where we use substr() and it'd be nice to use string_views directly).

About base::split_string, just in case there exists base::tok() function (in base/tok.h) which is a little faster and have some kind of different API if required.

The base::tok() function is based on https://github.com/dacap/tok, and there you can find some benchmarks too: https://github.com/dacap/tok/blob/main/benchmarks/tok_benchmarks.cpp
(I'd like to benchmark base::tok() using string_view some time in the future).

@dacap dacap merged commit 9cc226c into aseprite:beta Dec 5, 2024
11 checks passed
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.

3 participants