Bug 98705 - Reduce starting time on Windows by removing GetCaseCorrectPathName usage
Summary: Reduce starting time on Windows by removing GetCaseCorrectPathName usage
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All Windows (All)
: medium normal
Assignee: Kunal Pawar
URL: https://gerrit.libreoffice.org/q/tdf%...
Whiteboard: target:7.4.0
Keywords: difficultyMedium, easyHack, skillCpp, topicDebug
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2016-03-16 12:22 UTC by László Németh
Modified: 2022-04-06 13:21 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
Patch for resolving the Bug (5.06 KB, patch)
2017-03-24 10:36 UTC, aasthaagrrawal
Details
Patch for resolving the Bug (3.91 KB, patch)
2017-03-25 17:21 UTC, aasthaagrrawal
Details

Note You need to log in before you can comment on or make changes to this bug.
Description László Németh 2016-03-16 12:22:44 UTC
GetCaseCorrectPathName is a slow and likely obsolete function, mostly removed from the code.
Removing temporarily its last occurance (by the following test patch) resulted ~10% speed up in LO starting time on Windows.
So it would be fine to remove (or replace it GetLongPathName or something relevant, if needed) finally.

--- a/sal/osl/w32/file_dirvol.cxx
+++ b/sal/osl/w32/file_dirvol.cxx
@@ -1666,7 +1666,7 @@ oslFileError SAL_CALL osl_getFileStatus(
 
     if ( uFieldMask & osl_FileStatus_Mask_FileURL )
     {
-        if ( !pItemImpl->bFullPathNormalized )
+        if ( 0 && !pItemImpl->bFullPathNormalized )
         {
             ::osl::LongPathBuffer< sal_Unicode > aBuffer( MAX_LONG_PATH );
             sal_uInt32 nNewLen = GetCaseCorrectPathName( rtl_uString_getStr( pItemImpl->m_pFullPath ),

Some information in sal/osl/w32/file_url.cxx:

// Same as GetLongPathName but also 95/NT4
static DWORD GetCaseCorrectPathNameEx(
    LPWSTR  lpszPath,   // path buffer to convert
    DWORD   cchBuffer,      // size of path buffer
    DWORD   nSkipLevels,
    BOOL bCheckExistence )
Comment 1 jani 2016-03-17 07:33:27 UTC
Thanks for the solution, but please follow our guidelines for submitting patches:
https://wiki.documentfoundation.org/Development/GetInvolved

We use gerrit to review and apply patches.
Comment 2 Michael Meeks 2016-03-17 12:56:33 UTC
Jan - this is an easy-hack; the patch is just to highlight the win possible from optimizing the implementation here =) This is fundamentally a cleanup to use a built-in windows function instead of a hacked-up compatibility implementation.
Comment 3 aasthaagrrawal 2017-03-23 15:44:09 UTC
Hi, I am working on this bug.
Comment 4 aasthaagrrawal 2017-03-24 10:36:14 UTC
Created attachment 132121 [details]
Patch for resolving the Bug
Comment 5 Michael Meeks 2017-03-24 11:07:44 UTC
Thanks for the patch ! =) a few comments:

* can you submit it to gerrit ? - it takes a bit of setup - but good to get that done.

* can you remove the dead code rather than commenting it out ? if we had every line of code we'd removed over time still in the source - we would have 50x as much code and it would be un-readable & un-grep-able =)

Otherwise - this looks good; can you test your binaries on Windows XP ? and ensure that this method is triggered & continues to work ? I hope you can just copy your 'instdir' across and run it there.

Thanks !
Comment 6 aasthaagrrawal 2017-03-25 17:21:52 UTC
Created attachment 132143 [details]
Patch for resolving the Bug
Comment 7 Shinnok 2017-08-25 08:31:01 UTC
A polite ping, still working on this patch?
Comment 8 Shinnok 2017-08-28 12:05:50 UTC Comment hidden (obsolete)
Comment 9 Archit Jugran 2017-12-28 18:27:25 UTC
i Want to solve this bug as my first easy hack . i want to confirm whether i should simply remove all pieces of code where i find the function GetCaseCorrectPathName() called, or something more? .Thank you for your help in advance :)
Comment 10 Xisco Faulí 2018-01-24 13:48:52 UTC Comment hidden (obsolete)
Comment 11 Tor Lillqvist 2018-01-24 13:52:50 UTC
The "hack" to be done here is so trivial (especially when there already is a patch attached (that was ignored)), that any help would essentially mean doing the hack for him/her.
Comment 12 Shubham Verma 2018-02-25 20:12:54 UTC
Although the patch is there in the comments,written by someone else, but it has not been committed yet. I want to seek the permission to look into it for possible changes and correction if any error arises.
Comment 13 Michael Meeks 2018-02-26 11:26:26 UTC
Please do re-do the work yourself, and submit it to gerrit for testing if Archit  cannot do this =) Thanks !
Comment 14 Xisco Faulí 2018-03-30 02:32:16 UTC Comment hidden (obsolete)
Comment 15 Shubham Verma 2018-04-01 14:25:10 UTC
Sorry for the delay , I am still working on it.
Comment 16 Xisco Faulí 2018-05-02 02:31:15 UTC Comment hidden (obsolete)
Comment 17 Xisco Faulí 2018-06-02 03:07:12 UTC Comment hidden (obsolete)
Comment 18 Xisco Faulí 2018-07-03 02:36:52 UTC Comment hidden (obsolete)
Comment 19 Xisco Faulí 2018-08-03 02:49:44 UTC Comment hidden (obsolete)
Comment 20 Xisco Faulí 2018-09-03 02:40:38 UTC Comment hidden (obsolete)
Comment 21 Xisco Faulí 2018-10-04 02:53:17 UTC Comment hidden (obsolete)
Comment 22 Xisco Faulí 2018-11-04 03:55:26 UTC Comment hidden (obsolete)
Comment 23 Xisco Faulí 2018-12-05 03:47:06 UTC Comment hidden (obsolete)
Comment 24 Xisco Faulí 2019-01-05 03:39:55 UTC Comment hidden (obsolete)
Comment 25 Xisco Faulí 2019-02-05 03:44:19 UTC Comment hidden (obsolete)
Comment 26 Tor Lillqvist 2019-02-05 08:52:45 UTC Comment hidden (off-topic)
Comment 27 Tor Lillqvist 2020-02-25 12:59:58 UTC
See comment 11. For this "easy hack" to have any relevance at all any longer as an indication of hacking skills, I'd say that the easy-hacker is expected to do some measurements, too, to indicate how much a win doing it gives us...
Comment 28 Mike Kaganski 2020-04-06 11:54:58 UTC
This "easy hack" is not that easy. Anyone who would attempt to fix this, should not expect one's task to be limited to replacement of uses of the function with its Win32 counterpart API. These two actually have differences in behaviour, and we in fact depend on the specifics of GetCaseCorrectPathName (which makes the brute-force attempts replacing one with the other to fail in unit tests). The debugging of the unit tests failure cause, understanding the differences, documenting them (here), and investigating if dependencies on those differences might be fixed in the code is the minimum to hack on this.
Comment 29 Kunal Pawar 2022-02-18 13:49:55 UTC
I am trying to work on this
https://gerrit.libreoffice.org/c/core/+/130144
Comment 30 Michael Warner 2022-02-19 14:42:58 UTC
(In reply to Kunal Pawar from comment #29)
> I am trying to work on this
> https://gerrit.libreoffice.org/c/core/+/130144

Hello Kunal, thanks for taking this one! Since you are working on it, please set the Assignee field (at the top) to your email address, and set the status to ASSIGNED. This helps us with bug triage.
Comment 31 Commit Notification 2022-03-04 16:01:36 UTC
Kunal Pawar committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/92e835dbf00590c9c29509d2995cc7918a9bbb90

tdf#98705 Replace GetCaseCorrectPathName with GetLongPathNameW

It will be available in 7.4.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 Buovjaga 2022-04-06 13:21:27 UTC
Looks like this can be closed