Skip to content

Issue #506: task_isolation - rebase feature on the new branch, add unit tests, add Lazy wrapper#676

Open
olologin wants to merge 1 commit into
taskflow:masterfrom
ModuleWorks:task_isolation
Open

Issue #506: task_isolation - rebase feature on the new branch, add unit tests, add Lazy wrapper#676
olologin wants to merge 1 commit into
taskflow:masterfrom
ModuleWorks:task_isolation

Conversation

@olologin

@olologin olologin commented Apr 9, 2025

Copy link
Copy Markdown

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

bradphelan commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@olologin

olologin commented Apr 17, 2025

Copy link
Copy Markdown
Author

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

tsung-wei-huang commented May 7, 2025

Copy link
Copy Markdown
Member

@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