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

Cleanups: Simplify thread and condition_variable, identify unused dllexports #3532

Merged

Conversation

StephanTLavavej
Copy link
Member

This PR contains fairly simple logic transformations - they aren't exactly trivial (as one must consult other code to verify their correctness), but they're far from complicated. There's no behavioral impact, no changes to dllexports, and no interaction with the "unlocked" state of the redist.

  • First, _Xtime_diff_to_millis() is unused. We can drop its declaration in inc/xtimec.h and mark its definition in src/xtime.cpp as preserved for bincompat. When doing this, all we need to do is verify that the declaration and the definition contain all of the same annotations (in particular, _CRTIMP2_PURE controls dllexporting).
  • Simplify thread::detach(), part 1.
    • inc/xthreads.h provided an inline helper _Check_C_return(). Originally, it may have been used widely, but now there's only one usage. We can manually fuse it into thread::detach().
  • Simplify thread::detach(), part 2.
    • _Thrd_detach() returns either _Thrd_success or _Thrd_error:

      STL/stl/src/cthread.cpp

      Lines 71 to 73 in 16bb556

      int _Thrd_detach(_Thrd_t thr) { // tell OS to release thread's resources when it terminates
      return CloseHandle(thr._Hnd) ? _Thrd_success : _Thrd_error;
      }
    • _Throw_C_error() translates that to _Throw_Cpp_error(_INVALID_ARGUMENT):

      STL/stl/src/thread0.cpp

      Lines 47 to 48 in 16bb556

      case _Thrd_error:
      _Throw_Cpp_error(_INVALID_ARGUMENT);
    • Thus, we can simplify the logic even more. Now it resembles thread::join() above, and more clearly shows what exception will be thrown.
  • Simplify threads, part 3.
    • _Cnd_timedwait() returns only _Thrd_success or _Thrd_timedout. There's only one return, and only two values that res can take:

      STL/stl/src/cond.cpp

      Lines 63 to 83 in 16bb556

      int _Cnd_timedwait(const _Cnd_t cond, const _Mtx_t mtx, const xtime* const target) { // wait until signaled or timeout
      int res = _Thrd_success;
      const auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
      if (target == nullptr) { // no target time specified, wait on mutex
      _Mtx_clear_owner(mtx);
      cond->_get_cv()->wait(cs);
      _Mtx_reset_owner(mtx);
      } else { // target time specified, wait for it
      xtime now;
      xtime_get(&now, TIME_UTC);
      _Mtx_clear_owner(mtx);
      if (!cond->_get_cv()->wait_for(cs, _Xtime_diff_to_millis2(target, &now))) { // report timeout
      xtime_get(&now, TIME_UTC);
      if (_Xtime_diff_to_millis2(target, &now) == 0) {
      res = _Thrd_timedout;
      }
      }
      _Mtx_reset_owner(mtx);
      }
      return res;
      }
    • This allows us to simplify the calls to _Cnd_timedwait(), as we don't need to worry about unexpected return values.
    • After this, _Throw_C_error() is unused, so we can drop its declaration and mark its definition as preserved for bincompat.
    • There's one difference between the declaration and the definition, but we don't need the extern "C++" from the declaration in inc/xthreads.h - that's for Standard Library Modules, but src/thread0.cpp is always built classically.

_Thrd_detach() returns either _Thrd_success or _Thrd_error.

_Throw_C_error() translates that to _Throw_Cpp_error(_INVALID_ARGUMENT).
_Cnd_timedwait() returns only _Thrd_success or _Thrd_timedout, allowing us to simplify later code.

After this, _Throw_C_error() is unused. (No redist impact, still exported.)

We don't need the `extern "C++"` from the declaration in stl/inc/xthreads.h - that's for Standard Library Modules, but stl/src/thread0.cpp is always built classically.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Mar 3, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 3, 2023 21:03
@github-actions github-actions bot added this to Initial Review in Code Reviews Mar 3, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Mar 3, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Mar 7, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 7, 2023
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit f2633d3 into microsoft:main Mar 7, 2023
Code Reviews automation moved this from Ready To Merge to Done Mar 7, 2023
@StephanTLavavej StephanTLavavej deleted the stl-cleanups-unused-exports branch March 7, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants