Skip to content

Conversation

@olologin
Copy link

@olologin olologin commented Apr 9, 2025

Solves #506
Of course I just show my changes, and I expect some improvements must be made to upstream this. Let me know if you see some area for improvement.
Also, I had to rebase these changes from last release branch, so maybe I missed something. But for our internal usage (which is mostly Lazy class) it works fine. We did not see significant slowdowns because of additional operations in scheduler yet.

Would be great to upstream this eventually.

Brief explanation of how this works: #506 (comment)

@bradphelan
Copy link
Contributor

bradphelan commented Apr 10, 2025

I like this very much. I would be tempted to clean the lazy class by extracting a ValueOrException class. The inling of the placement new and other related machinery next to the taskflow machinery makes understanding either more complex. They can be seperated. I have two before and after examples.

Before: https://godbolt.org/z/qsEbf569q

After: https://godbolt.org/z/dr175s35q

( Note on Godbolt only taskflow trunk is available so I commented out the code that uses isolate )

The extraction is a simple class which can be tested in isolation. Perhaps the name of the class should be TryCatch because that is really what it is modelling.

// New helper class that encapsulates value or exception handling
template <typename T>
class ValueOrException {
  alignas(std::max(alignof(T), alignof(std::exception_ptr)))
  unsigned char m_data[std::max(sizeof(T), sizeof(std::exception_ptr))];
  enum class State { NONE, VALUE, EXCEPTION } m_state = State::NONE;

public:
  ~ValueOrException() {
    reset();
  }

  /// Emplaces the result of calling fn directly into
  /// the store. If the function fails then the exception
  /// is stored.
  void emplace_fn(std::function<T()> fn) {
    try {
      new (m_data) T(fn());
      m_state = State::VALUE;
    } catch (...) {
      new (m_data) std::exception_ptr(std::current_exception());
      m_state = State::EXCEPTION;
    }
  }

  T* get() {
    if (m_state == State::EXCEPTION) {
      std::rethrow_exception(*std::launder(reinterpret_cast<std::exception_ptr*>(m_data)));
    }
    return std::launder(reinterpret_cast<T*>(m_data));
  }

  bool has_value() const {
    return m_state == State::VALUE;
  }

  bool ready() const {
    return m_state != State::NONE;
  }

  void reset() {
    if (m_state == State::VALUE) {
      std::launder(reinterpret_cast<T*>(m_data))->~T();
    } else if (m_state == State::EXCEPTION) {
      std::launder(reinterpret_cast<std::exception_ptr*>(m_data))->~exception_ptr();
    }
    m_state = State::NONE;
  }
};

@tsung-wei-huang
Copy link
Member

Thank you very much for this pull request - let me look into this and see how we can make it better!

@olologin
Copy link
Author

olologin commented Apr 17, 2025

Not sure about the implementation, but regarding API:
I think it would be better to get rid of Executor::isolate method and allow user to use TaskArenaApplierRAII directly (maybe with better name).

This way user can implement some kind of cooperative mutex locking that does not block the calling thread and instead runs corun_until until mutex is available.
Basically it would be an alternative like

taskflow::Executor ex;
std::mutex m;
...
taskflow::scoped_lock l(m, ex); // If m.lock() succeeds - enter new task arena for the current thread, if not - ex.corun_until until m.lock() succeeds.

// Code that requires single-threaded execution
...

@tsung-wei-huang
Copy link
Member

tsung-wei-huang commented May 7, 2025

@olologin , I am looking into this pull - and thank you again for this great contribution. A few suggestions:

  1. Can you rebase it to the current master branch that reflects the newest release 3.10? We incorporated quite a few performance-related changes to executor, but I think those changes do not have much conflict with your TaskArena proposal.
  2. I suggest using std::enable_shared_from_this for TaskArena, which makes it more clear to understand the shared ownership of TaskArena.
  3. I suggest renaming TaskArenaApplierRAII to ScopedTaskArena or TaskArenaGuard to better align with C++ naming convention.
  4. I agree with @bradphelan 's suggestion of ValueOrException

Feel free to let me know if you have any thoughts. Thank you very much for this pull again!

@tsung-wei-huang tsung-wei-huang added the enhancement New feature or request label May 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants