Bug 97087 - Timers and idles should have programmer comprehensible, unique names
Summary: Timers and idles should have programmer comprehensible, unique names
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.1.0.1 rc
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.3.0 target:5.4....
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2016-01-12 22:41 UTC by Michael Meeks
Modified: 2021-02-12 16:33 UTC (History)
5 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 2016-01-12 22:41:34 UTC
We have a lot of Idle and Timer objects inside LibreOffice that drive lots of logic. We can associate a const char *pDebugName with these as we create them that really helps during debugging of the scheduler cf.

    Idle( const sal_Char *pDebugName = nullptr );
...
    Timer( const sal_Char *pDebugName = nullptr );

It would be great to annotate each timer with a short, unique (ideally 10-15chars or less) C string that briefly describes it. 

Thanks !
Comment 1 Michael Meeks 2016-01-12 22:42:39 UTC
We should also fix this typo; 'git grep' for it:

"DoYield with no ides returns: timeout" -> "no idles"
Comment 2 Robinson Tryon (qubit) 2016-02-18 14:52:08 UTC Comment hidden (obsolete)
Comment 3 Commit Notification 2016-04-07 20:50:08 UTC
Muhammet Kara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e943da4189e829c7b65b6d171bd6d16ca5e0b425

Fix simple typo in SAL_INFO tdf#97087

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 4 Commit Notification 2016-04-07 20:52:54 UTC
Muhammet Kara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e88ba02a70c3f2279ed7ceefeb94f843537bcf8e

tdf#97087 - Give Idles comprehensible and unique names

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 5 Michael Meeks 2016-04-11 08:45:26 UTC
Hi Muhammet - the strings are understandable, but not that unique eg.

+        mpOnlineSpellingIdle = new Idle("OnlineSpelling");

I would prefer "OnlineSpelling (Draw)" or something =) since there will be similar functionality for spell checking in many places. Similarly:

+        pPageImpl->pLoadIdle = new Idle("DelayedLoad");

could be "Background Tab Page - delayed load".

It is best if the strings are unique and this grep-able in the code-base =)

Otherwise - good stuff.
Comment 6 Commit Notification 2016-04-13 08:08:48 UTC
Muhammet Kara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c03a11b8f0c93b1c55d9abc9aa224aeb298d1976

tdf#97087 Give comprehensible names to timers

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 7 Muhammet Kara 2016-04-13 10:58:45 UTC
(In reply to Michael Meeks from comment #5)
> Hi Muhammet - the strings are understandable, but not that unique eg.
> 
> +        mpOnlineSpellingIdle = new Idle("OnlineSpelling");
> 
> I would prefer "OnlineSpelling (Draw)" or something =) since there will be
> similar functionality for spell checking in many places. Similarly:
> 
> +        pPageImpl->pLoadIdle = new Idle("DelayedLoad");
> 
> could be "Background Tab Page - delayed load".
> 
> It is best if the strings are unique and this grep-able in the code-base =)
> 
> Otherwise - good stuff.

I forgot to add myself to cc. That's why I am reading your comment now. Sorry about that, and for the noise. :)
Comment 8 Muhammet Kara 2016-04-14 06:43:01 UTC
Should we also name the idles in header files / class definitions? Like this one: http://opengrok.libreoffice.org/xref/core/cui/source/inc/linkdlg.hxx#65
Comment 9 Michael Meeks 2016-04-14 09:07:25 UTC
class Foo {
...
    Idle aUpdateIdle;
...
    Foo();
...
}.

It is unclear how you'll initialize that in the header ? =) it needs to be initialized in the constructor:

Foo::Foo() :
    ...
    aUpdateIdle("foo baa update idle"),
    ...
{
...
}

Surely ? =) but the answer is yes - we want all of these tagged with a human readable, debug name that can be used to work out what is going on in the scheduler =)

Thanks !
Comment 10 Commit Notification 2016-04-28 09:36:29 UTC
Muhammet Kara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=376c337d46acf8819bf032251bfc7d5eb31db198

tdf#97087 Give comprehensible, unique names to idles

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 jani 2016-04-29 06:08:37 UTC
Seems solved
Comment 12 Muhammet Kara 2016-05-01 07:20:43 UTC
(In reply to jan iversen from comment #11)
> Seems solved

Not yet. There are still a lot of those variables to be named.
Comment 13 Muhammet Kara 2016-05-01 07:40:08 UTC
Some tips for the ones who would like to work on this bug:

- Make sure all the names you give are unique
- You don't need to add "Idles" to all names of idles because they can be easily identified from the scheduler
- Be careful about the order of initializers in constructors. Initialization must be done in the same order as the member variables was declared in the header file.
- You may review the previous patches to see more.
Comment 14 Commit Notification 2016-05-16 01:09:22 UTC
Muhammet Kara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=98505584ba3f324e36800704bbdacc2b7cf0ef71

Give unique, comprehensible names to idles tdf#97087

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 15 Aparajita Haldar 2016-06-10 05:41:21 UTC
(In reply to Muhammet Kara from comment #13)
> Some tips for the ones who would like to work on this bug:
> 
> - Make sure all the names you give are unique
> - You don't need to add "Idles" to all names of idles because they can be
> easily identified from the scheduler
> - Be careful about the order of initializers in constructors. Initialization
> must be done in the same order as the member variables was declared in the
> header file.
> - You may review the previous patches to see more.

Hello!
I'd like to get involved with the open source community. Would this fix be a reasonable place to make my first contribution?
Comment 16 Muhammet Kara 2016-06-10 13:32:31 UTC
(In reply to Aparajita Haldar from comment #15)
> (In reply to Muhammet Kara from comment #13)
> 
> Hello!

Hi Aparajita!

> I'd like to get involved with the open source community. Would this fix be a
> reasonable place to make my first contribution?

Good to hear that. Why not. An easyhack is a good place to start.

opengrok.libreoffice.org and "git grep" are your friends to find idles/timers which are not named or not initialized in the constructors.
Comment 17 jani 2016-06-11 12:04:46 UTC
> Hello!
> I'd like to get involved with the open source community. Would this fix be a
> reasonable place to make my first contribution?

Hi you are most welcome to help with solving this bugs (and others), if you have C++ experience this is an easy one.

We have made a step by step guide, to help you successfully get your first patch merged:

https://wiki.documentfoundation.org/Development/GetInvolved

Looking forward to see patches from you.
rgds
jan I.
Comment 18 Commit Notification 2016-06-11 17:18:36 UTC
apurvapriyadarshi committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b135ba875a22a86d89f25bbc229cf2b7edcbe8c

tdf#97087 Timers and idles should have programmer comprehensible, unique names

It will be available in 5.3.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 2016-06-13 15:34:15 UTC
emahaldar/em committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e8dfe9bca7c2430df9d231ec79ed540e45f18789

Give unique, comprehensible names to timers tdf#97087

It will be available in 5.3.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 jani 2016-06-14 10:02:10 UTC
Controlled, still open
Comment 21 Gökhan Gurbetoğlu 2016-06-15 13:34:36 UTC
I have annotated all idles in the codebase and waiting for code review. I am now switching to annotating the timers.
Comment 22 Tor Lillqvist 2016-06-16 07:43:36 UTC
I have a better idea about naming Timer and Idle instances:

Let's not try to come up manually with names at all. What is the intended use case for these names, after all? 

Isn't it like this: you see in SAL_INFO output some information like "timer 'Mumble Frobozzinication Timer' fired". What is the next thing you do? You git grep for "Mumble Frobozzinication Timer" to find the place where it is created and then search more around that for what is done to it.

Why not get rid of that 'git grep' stage and directly encode the file name, line number, and instance count in the name of the timer?

We would need some function like comphelper::MakeUniqueInstanceId(const char **file, int line) that would be called where Timer objects are constructed like MakeUniqueInstanceId(__FILE__, __LINE__).

It and would generate a string like "sc-source-foo-bar-mumble-cxx-123-4", where 123 would be the line number, and 4 a counter of how many times a timer has been constructed at that line.
Comment 23 Tor Lillqvist 2016-06-16 07:53:30 UTC
Another thing: We should change the name field to be an OUString. Using a const char * is just fallback to C thinking and micro-optimisation that leads to never-ending joy of lifecycle and/or leak issues, as surely it will be necessary/useful to name timers using a dynamically constructed name?
Comment 24 Michael Meeks 2016-06-16 08:52:48 UTC
Hi Tor,

> Another thing: We should change the name field to be an OUString. Using a const
> char * is just fallback to C thinking and micro-optimisation

I disagree. The joy of a char * is that we can pass a NULL pointer in the non-debug case - without needing any macro horrors; and avoid calling a pointless empty OUString constructor =)

    OUString()
    {
        pData = NULL;
        rtl_uString_new( &pData );
    }

Thanks,

Michael.

PS. - I see no problem with merging the existing patches - and then updating them with a new easy hack to Tor's scheme =)
Comment 25 Tor Lillqvist 2016-06-16 09:08:42 UTC
And how many millions of timers and idles do we have again so that it makes any difference?
Comment 26 Commit Notification 2017-01-17 15:25:52 UTC
Jan-Marek Glogowski committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9e51007039770370182839846676b205f5c34c57

tdf#97087 GDB pretty print the Scheduler task list

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 27 Commit Notification 2017-04-09 15:02:14 UTC
blendergeek committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=886d76e50eab863a38010b983214e2ecaca658cf

tdf#97087, Add debug name to an Idle object

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 28 Mariam Fahmy 2020-10-15 20:14:49 UTC
Hello, I'd like to start contributing in the open source community, any advice to solve this bug will be very helpful as I am beginner.
Thanks in advance
Comment 29 Homeboy_445 2021-01-11 16:23:56 UTC
Please review the changes made by me:https://gerrit.libreoffice.org/c/core/+/109056
Comment 30 Jan-Marek Glogowski 2021-01-13 23:14:29 UTC
Just some reminder. There is the additional API of SetDebugName, which does the same as the constructors. So if you use the constructor, check there isn't already a SetDebugName call. And there is a SetIdleDebugName, which forwards the name from a subclass.
Comment 31 Commit Notification 2021-01-16 11:19:11 UTC
homeboy445 committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5b4486a898968eb5019376ca6c4bd07a97ec537d

tdf#97087 Give unique, comphrehensible names to idles

It will be available in 7.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 32 CHAYAN DAS 2021-02-08 18:08:35 UTC
Hello!
I'd like to get involved with the open source community. Would this fix be a reasonable place to make my first contribution?
Comment 33 CHAYAN DAS 2021-02-08 18:09:47 UTC
I am not understanding from where to get started, confused
Comment 34 Buovjaga 2021-02-09 14:56:21 UTC
This is a multi-hacker task, please don't assign it to yourself
Comment 35 indrajeet sinha 2021-02-12 11:24:55 UTC
hey i would like to work on this issue
Comment 36 Rakshita Varadarajan 2021-02-12 16:33:58 UTC
Hello! I just submitted a patch for this, I don't know why the automatic bug-notification was not generated even though I followed protocol.
Anyways, kindly do review the patch! Link: https://gerrit.libreoffice.org/c/core/+/110827