| Summary: | sal - add sane sleep interface: cleanup osl_waitThread | ||
|---|---|---|---|
| Product: | LibreOffice | Reporter: | Michael Meeks <michael.meeks> |
| Component: | LibreOffice | Assignee: | Kevin Dubrulle <kevin.dubrulle> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | h3734236, mentoring, sberg.fun |
| Priority: | medium | Keywords: | difficultyBeginner, easyHack, skillCpp, topicCleanup |
| Version: | 4.3.0.2 rc | ||
| Hardware: | Other | ||
| OS: | All | ||
| See Also: | https://bugs.documentfoundation.org/show_bug.cgi?id=100085 | ||
| Whiteboard: | target:5.2.0 target:5.4.0 target:6.2.0 | ||
| Crash report or crash signature: | Regression By: | ||
Easy-hackify ... Hi I would like to know what I suppose to do in this part: And then we should fix the call-sites that cooked their own version; even inside sal/ this happens: cd sal ; git grep -5 Sleep to find the several instances that need to be cleaned up here =) Should I replaces the places where Sleep appears to wait or usleep or should I just remove Sleep. Thanks We should do a functionally identical replacement of Sleep with our nice new, static / inline (?) usleep thing I guess: re-factoring & cleanup should provide something functionally identical in that commit. Perhaps later we'll discover that we can do better and/or drop some sleeps too =) Workin' on that. Stephan Bergmann committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=b190574b6a0de3e26c6338324f74440ec0505bfb tdf#84323: Make osl::Thread::wait more readable It will be available in 5.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. Stephan Bergmann committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=26821b7e2eff4a7db22606f70daa3f3d442f7525 tdf#84323: Make osl::Condition::wait more readable It will be available in 5.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. See the commits mentioned in comment 5 and comment 6 for ways to improve the readability of calls to osl::Thread::wait and osl::Condition::wait, respectively. Improve further calls of these two functions across the code base. Migrating Whiteboard tags to Keywords: (easyHack, difficultyBeginner, skillCpp, topicCleanup) [NinjaEdit] JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit] Gurkaran committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=9e7447d39e356857ef5786b513e99cee79385247 tdf#84323: Make osl::Thread::wait more readable It will be available in 5.2.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. Gurkaran committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=d27e3eca2cca1249a0bdd9e6385ca693d471ef9b tdf#84323: Make osl::Condition::wait more readable It will be available in 5.2.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. (asked at <https://gerrit.libreoffice.org/#/c/23417/2> to clarify whether comment 10 and comment 11 cover all calls that remained to be cleaned up, in which case this issue should be closed now) Some good improvements here; I hate to re-open bugs; but a quick grep shows eg.
helpcompiler/source/HelpCompiler.cxx-static void impl_sleep( sal_uInt32 nSec )
helpcompiler/source/HelpCompiler.cxx-{
helpcompiler/source/HelpCompiler.cxx- TimeValue aTime;
helpcompiler/source/HelpCompiler.cxx- aTime.Seconds = nSec;
helpcompiler/source/HelpCompiler.cxx- aTime.Nanosec = 0;
helpcompiler/source/HelpCompiler.cxx-
helpcompiler/source/HelpCompiler.cxx: osl::Thread::wait( aTime );
helpcompiler/source/HelpCompiler.cxx-}
Which is another victim of this API ;-)
I'd love to cleanup and clarify things like:
sal/qa/osl/thread/test_thread.cxx- // Give the spawned threads enough time to terminate:
sal/qa/osl/thread/test_thread.cxx- TimeValue const twentySeconds = { 20, 0 };
sal/qa/osl/thread/test_thread.cxx: osl::Thread::wait(twentySeconds);
or:
sd/qa/unit/misc-tests.cxx- TimeValue aSleep(0, 100 * 1000000); // 100 msec
sd/qa/unit/misc-tests.cxx: osl::Thread::wait(aSleep);
Which could be much clearer with the chrono thing =)
Also I would want to see a nice example in include/sal if possible - in a verbose comment - that includes the word 'sleep' ;-)
cd include/sal
git grep -i sleep
still no hits =)
Hope that makes sense. Any chance of some more cleanup there ? =)
A polite ping, still working on this issue ? Would be good to turn comment 13 into a new easy-hack perhaps =) (In reply to Michael Meeks from comment #15) > Would be good to turn comment 13 into a new easy-hack perhaps =) As suggested so done :-) https://bugs.documentfoundation.org/show_bug.cgi?id=100085 Unassign due to lack of work. Remark, if you want to continue working on this patch, please assign it again. Fakabbir Amin committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=12fc1d5399a688a80eec2565a4b552377e428ab7 tdf#84323: Make osl::Condition::wait more readable It will be available in 5.4.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. dilekuzulmez committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=8468945322b72ddadbe42584b9bba395c1e5bb7e tdf#84323: Make osl::Condition::wait more readable It will be available in 5.4.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. dilekuzulmez committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=cbda84e5422c066b93d8c06b49c6bc86eca7c71f tdf#84323: Make osl::Condition::wait more readable It will be available in 5.4.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. A polite ping, still working on this bug? Hi, I would like to work on that. First, I am cleanning calls to osl_waitThread with a hand made TimeValue with std::chrono. After, I will replace osl_waitThread to osl::Thread::wait, where it is possible. A polite ping, still working on this bug? Hi, Didn't have time recently, but I will ! Kevin Dubrulle committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=d0f44d8ba7e87aa263008d3cfc4e68294d783162 tdf#84323 - sal - add sane sleep interface: cleanup osl_waitThread It will be available in 6.2.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback. A polite ping, still working on this bug? Hi, In my opinion, this issue should be closed. Sounds sensible - thanks for the nice fixes everyone ! =) |
LibreOffice has a nice 'usleep' style function - but its docs suck. the method is osl_waitThread - and it is sufficiently opaque that I couldn't find it: include/osl/thread.h /** Blocks the calling thread at least for the given number of time. */ SAL_DLLPUBLIC void SAL_CALL osl_waitThread(const TimeValue* pDelay); That comment needs to be longer, describe the parameter, and mention the string 'sleep' and also 'usleep' ideally =) It might be nice to have a usleep version that sets up the TimeValue for you in the hxx header too: static void SAL_CALL wait(const TimeValue& Delay) { osl_waitThread(&Delay); } And then we should fix the call-sites that cooked their own version; even inside sal/ this happens: cd sal ; git grep -5 Sleep to find the several instances that need to be cleaned up here =) Thanks !