Comment by malkia

5 years ago

Since perf is at utmost importance for this project, and intern has been found to be used a lot, maybe a pinch of small optimization is to move the static ConcurrentMap out of the function, hence avoid atomic<bool> check on whether it's initialized -

  static Symbol *intern(std::string_view name) {
    static ConcurrentMap<Symbol> map;
    return map.insert(name, {name});
  }

to

  static ConcurrentMap<Symbol> map;

  static Symbol *intern(std::string_view name) {
    return map.insert(name, {name});
  }

probably though it won't bring much, but for the sake of squeezing every bit out there (and it was the easiest I could find - lol)

7 comments

malkia

Reply
malkia  5 years ago

Wait this code can't work - as you holding only a string_view in the Symbol...

  // __start_ and __stop_ symbols
  for (OutputChunk *chunk : chunks) {
    if (is_c_identifier(chunk->name)) {
      start(Symbol::intern("__start_" + std::string(chunk->name)), chunk);
      stop(Symbol::intern("__stop_" + std::string(chunk->name)), chunk);
    }
  }

  • ArchOversight  5 years ago

    This does seem broken, since the temporary string that is created by the concatenation is short lived, and the std::string_view inside the Symbol won't hold on to it, just the pointer to the data.

  • malkia  5 years ago

    Or this:

    for (std::string_view arg : config.version_script) parse_version_script(std::string(arg));

    • malkia  5 years ago

      Although if somehow nothing gets really freed, and std::string() is kept intact after calling the destructor (e.g. it's data() is still valid) then it'll work :)

    • ArchOversight  5 years ago

      `parse_version_script` is defined as taking an `std::string`, std::string() over an `std::string_view` will create a new std::string with a copy of the data from arg.

      1 reply →