Bug 84323 - sal - add sane sleep interface: cleanup osl_waitThread
Summary: sal - add sane sleep interface: cleanup osl_waitThread
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.3.0.2 rc
Hardware: Other All
: medium normal
Assignee: Kevin Dubrulle
URL:
Whiteboard: target:5.2.0 target:5.4.0 target:6.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-09-25 10:05 UTC by Michael Meeks
Modified: 2018-08-22 08:11 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2014-09-25 10:05:57 UTC
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 !
Comment 1 Michael Meeks 2014-09-25 10:06:38 UTC
Easy-hackify ...
Comment 2 andreyai 2014-11-02 13:03:25 UTC
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
Comment 3 Michael Meeks 2014-11-03 10:09:04 UTC
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 =)
Comment 4 Michał Świętek 2015-03-25 11:17:24 UTC
Workin' on that.
Comment 5 Commit Notification 2015-08-17 15:38:12 UTC
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.
Comment 6 Commit Notification 2015-08-18 06:23:07 UTC
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.
Comment 7 Stephan Bergmann 2015-08-18 06:35:23 UTC
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.
Comment 8 Robinson Tryon (qubit) 2015-12-13 11:04:04 UTC Comment hidden (obsolete)
Comment 9 Robinson Tryon (qubit) 2016-02-18 14:52:02 UTC Comment hidden (obsolete)
Comment 10 Commit Notification 2016-03-22 06:56:48 UTC
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.
Comment 11 Commit Notification 2016-03-22 06:59:26 UTC
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.
Comment 12 Stephan Bergmann 2016-03-22 08:38:24 UTC
(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)
Comment 13 Michael Meeks 2016-03-22 10:35:38 UTC
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 ? =)
Comment 14 jani 2016-05-27 07:21:08 UTC
A polite ping, still working on this issue ?
Comment 15 Michael Meeks 2016-05-27 08:38:34 UTC
Would be good to turn comment 13 into a new easy-hack perhaps =)
Comment 16 jani 2016-05-27 09:07:32 UTC
(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
Comment 17 jani 2016-06-28 06:14:47 UTC
Unassign due to lack of work.

Remark, if you want to continue working on this patch, please assign it again.
Comment 18 Commit Notification 2017-02-20 08:58:48 UTC
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.
Comment 19 Commit Notification 2017-04-26 09:41:37 UTC
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.
Comment 20 Commit Notification 2017-04-26 10:01:18 UTC
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.
Comment 21 Shinnok 2017-08-25 13:14:58 UTC Comment hidden (obsolete)
Comment 22 Kevin Dubrulle 2018-05-27 12:45:15 UTC
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.
Comment 23 Xisco Faulí 2018-06-27 02:45:40 UTC Comment hidden (obsolete)
Comment 24 Kevin Dubrulle 2018-06-27 17:34:54 UTC
Hi,

Didn't have time recently, but I will !
Comment 25 Commit Notification 2018-07-08 09:49:37 UTC
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.
Comment 26 Xisco Faulí 2018-08-08 02:35:08 UTC Comment hidden (obsolete)
Comment 27 Kevin Dubrulle 2018-08-22 03:53:19 UTC
Hi,

In my opinion, this issue should be closed.
Comment 28 Michael Meeks 2018-08-22 08:11:04 UTC
Sounds sensible - thanks for the nice fixes everyone ! =)