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 !
We should also fix this typo; 'git grep' for it: "DoYield with no ides returns: timeout" -> "no idles"
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
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.
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.
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.
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.
(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. :)
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
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 !
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.
Seems solved
(In reply to jan iversen from comment #11) > Seems solved Not yet. There are still a lot of those variables to be named.
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.
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.
(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?
(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.
> 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.
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.
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.
Controlled, still open
I have annotated all idles in the codebase and waiting for code review. I am now switching to annotating the timers.
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.
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?
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 =)
And how many millions of timers and idles do we have again so that it makes any difference?
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.
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.
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
Please review the changes made by me:https://gerrit.libreoffice.org/c/core/+/109056
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.
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.
Hello! I'd like to get involved with the open source community. Would this fix be a reasonable place to make my first contribution?
I am not understanding from where to get started, confused
This is a multi-hacker task, please don't assign it to yourself
hey i would like to work on this issue
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
Tarun Sharma committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/2e417a09b5497bb8aa86727a280960aeb5023af0 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.
Looks like Noel solved the rest with https://git.libreoffice.org/core/commit/d72511eda923c827a6175bec9b8f24c237f82730 so let's close