Author Topic: Bug Report: [#18755] C::B hangs for 20 seconds while opening large project...  (Read 62964 times)

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Come up with a patch that works! :-)
Well here I got one: If combines Jens approach with my fix for UNC path's in the project tree.

This fixes all issues with presenting except this one:
http://forums.codeblocks.org/index.php/topic,16967.msg116003.html#msg116003

...but we are one step closer!
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
...Jens and others: before you start digging into it - I guess I've solved the last issue meanwhile. Just need some more testing, then I'll come up with a new patch.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
...here it comes, the final patch that works for all test cases (including different project tree viewer setups, btw):

The bottleneck: This fast method to obtain the path of a project file relative to the common top level path:
Code
//            f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
...does not work. I've commented why it does not work an use now also a wxFileName call. This is more expensive, but safe. I would love to see time measuring again, if it doesn't cost too much.

But I believe with this patch, we finally solved the actual issue with UNC, relative and path's on different drives all together once and for all.

Ps.: If you use a good diff-viewer, you can disable white space changes, as there are some not related to the actual fix. Sorry for that, I "a-style"d some portions for better understanding of what's going on.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Code
+        if (   (!prjHasUNCName &&  fileHasUNCName)
+            || ( prjHasUNCName && !fileHasUNCName) )

Maybe I'm a little pedantic here, but couldn't that be written as...

Code
if(prjHasUNCName != fileHasUNCName)

?

Anyway, here's the core of your patch:

Code
@@ -358,12 +380,37 @@
     for (FilesList::iterator it = m_Files.begin(); it != m_Files.end(); ++it)
     {
         ProjectFile* f = (*it);
-
         if (!f)
             continue;
 
         wxString fileName = f->file.GetFullPath();
-        f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
+        fileHasUNCName = fileName.StartsWith(_T("\\\\"));
+
+        if (   (prjHasUNCName && fileHasUNCName)
+            || (   !prjHasUNCName
+                && !fileHasUNCName
+                && vol.IsSameAs(f->file.GetVolume()) ) )
+        {
+            wxFileName relFileCTLP(f->file);
+            relFileCTLP.MakeRelativeTo( m_CommonTopLevelPath );
+            wxFileName relFileBase(f->file);
+            relFileBase.MakeRelativeTo( GetBasePath() );
+
+            // The commented (old) method to obtain the relativeToCommonTopLevelPath is fast, but does *not* work, if you save
+            // the project on a different drive in a sub-folder of an existing source file on that (different) drive:
+            // I.e.: Project on C:\Folder\Project.cbp has file C:\Folder\SubFolder\foo.cpp and D:\Folder\bar.cpp
+            // Saved the project under D:\Folder\SubFolder\ProjectNew.cbp would cause a wrong computation of bar.cpp otherwise!!!
+//            f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
+            // Using wxFileName instead, although its costly:
+            f->relativeToCommonTopLevelPath = relFileCTLP.GetFullPath();
+            f->relativeFilename             = relFileBase.GetFullPath();
+        }
+        else
+        {
+            f->relativeToCommonTopLevelPath = fileName;
+            f->relativeFilename             = fileName;
+        }
+

It looks pretty good to me; It uses wxFilename only when necessary. Anyway, I'm thinking that these two lines

Code
+            f->relativeToCommonTopLevelPath = relFileCTLP.GetFullPath();
+            f->relativeFilename             = relFileBase.GetFullPath();

could be optimized further by adding a path "cache" and comparing the file's relative pathname (the path without the filename) with the last file's pathname, and using the last calculated relativeToCommonTopLevelPath and relativeFilename instead of just calling wxFilename right away. This was my original proposal, btw, but all the other optimizations submitted here seem pretty rad :)

EDIT: OH, WAIT A MINUTE!!! I re-read that patch again...

Code
+        if (   (prjHasUNCName && fileHasUNCName)
+            || (   !prjHasUNCName
+                && !fileHasUNCName
+                && vol.IsSameAs(f->file.GetVolume()) ) )
+        {
This is the precondition for the fast calculation, right? And then you're just undoing all the optimizations here! If the only failing case happens when the old project's drive is different than the current one, why not add a condition that specifically says so?

Code
if(prjHasChangedDrives) { // prjHasChangedDrives is the real issue here, isn't it? 
//   If it has either changed windows drives or UNC network drives, we should use the full method;
//   Otherwise, the fast one.
//   Calculating prjHasChangedDrives should be trivial, anyway.

     wxFileName relFileCTLP(f->file);
     relFileCTLP.MakeRelativeTo( m_CommonTopLevelPath );
     wxFileName relFileBase(f->file);
     relFileBase.MakeRelativeTo( GetBasePath() );
     f->relativeToCommonTopLevelPath = relFileCTLP.GetFullPath();
     f->relativeFilename             = relFileBase.GetFullPath();
} else {
     f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
}
« Last Edit: October 31, 2012, 02:26:56 pm by rickg22 »

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
Another possibility:

Code
int fn_length = fileName.Length();
int prj_length = m_CommonTopLevelPath.Length();
bool is_file_above_project = fn_length <= m_CommonTopLevelPath.Length;
// This is an approximation; The correct approach would be not to compare the filenames, but the pathnames without filenames.
if(is_file_above_project) {  // use wxFilename here because the file is on a directory above the project
  wxFileName relFileCTLP(f->file);
  relFileCTLP.MakeRelativeTo( m_CommonTopLevelPath );
  wxFileName relFileBase(f->file);
  relFileBase.MakeRelativeTo( GetBasePath() );
  f->relativeToCommonTopLevelPath = relFileCTLP.GetFullPath();
  f->relativeFilename             = relFileBase.GetFullPath();
} else {
  // Also, shouldn't we compare the part of the path we're removing?
  f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
}

Approach 2:

Code
  int len_diff = fileName.Length() - m_CommonTopLevelPath.Length();
  bool path_mismatch = (len_diff < 0);
  if(!path_mismatch) {
     // If there's a wxString function that compares from right to left, we should use it here.
     path_mismatch = fileName.Left(m_CommonTopLevelPath.Length()) != m_CommonTopLevelPath;
     // NOTE: I'm assuming that m_CommonTopLevelPath ends in a directory separator; otherwise,
     // we could have problems if m_CommonTopLevelPath is something like "MyDirectory"
     // and the file is something like "MyDirectoryFilename".
  }
  if(path_mismatch) {
      wxFileName relFileCTLP(f->file);
      relFileCTLP.MakeRelativeTo( m_CommonTopLevelPath );
      wxFileName relFileBase(f->file);
      relFileBase.MakeRelativeTo( GetBasePath() );
      f->relativeToCommonTopLevelPath = relFileCTLP.GetFullPath();
      f->relativeFilename             = relFileBase.GetFullPath();
  } else {
      f->relativeToCommonTopLevelPath = fileName.Right(fileName.Length() - m_CommonTopLevelPath.Length());
  }
« Last Edit: October 31, 2012, 02:54:35 pm by rickg22 »

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Another possibility:
Rick, the problem is not switching the drive, but saving the project file below a folder of source files. This can be even on the same drive. So your patches re-introduce this bug.

Edit: Out posts crossed. I am inspecting your second post.
« Last Edit: October 31, 2012, 03:15:01 pm by MortenMacFly »
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
OK - after inspecting this, both of your methods are not safe.

The first one might fail, if the file name is longer than the path, for example (which can happen often) and the second one might fail as you have described in the comment already.

So - do we want something that works, or something that might work but is speedy? I prefer the first.

Also, remember that the speed-up was actually in another part of the code, so maybe we don't have issues at all and we do nut-picking. With this (my) patch applied, did you do time measuring with your project in question? IS it really that slow??? How much time does it take, compared to the initial version?
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
OK - after inspecting this, both of your methods are not safe.

The first one might fail, if the file name is longer than the path, for example (which can happen often) and the second one might fail as you have described in the comment already.

Yeah. I'd go for the second approach, the only thing I need is a function analogue to php's dirname() to solve any inconsistencies.

Quote
Also, remember that the speed-up was actually in another part of the code, so maybe we don't have issues at all and we do nut-picking. With this (my) patch applied, did you do time measuring with your project in question? IS it really that slow??? How much time does it take, compared to the initial version?

No, I haven't had time to test it, I may do it at home tonight. But if I'm correct, your fix will introduce exactly the same slowdown that was fixed in the other part of the code.

Anyway, against which version of SVN was the patch created?
« Last Edit: October 31, 2012, 10:26:54 pm by rickg22 »

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
But if I'm correct, your fix will introduce exactly the same slowdown that was fixed in the other part of the code.
I believe the opposite... but lets see... ;-)

Anyway, against which version of SVN was the patch created?
Trunk, as usual.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
By the way, this bug may be related to the wxFilename slowdown:

http://trac.wxwidgets.org/ticket/13915

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
But if I'm correct, your fix will introduce exactly the same slowdown that was fixed in the other part of the code.
I believe the opposite... but lets see... ;-)

14 seconds. :-/

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
14 seconds. :-/
Dammed. But I don't get it: According Jens and dmoore this patch:
http://forums.codeblocks.org/index.php/topic,16967.msg115619.html#msg115619
Did the trick. And this is not in the method we are touching now. :-\

Can I make a proposal: Shall we first commit what we have because it fixes the major bugs in file handling in project files. Then we think about what to do to speed-up things. The thing is I would like to freeze a working state as a reference before we fiddle with it again...
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline rickg22

  • Lives here!
  • ****
  • Posts: 2283
OK, I agree. Let's freeze this for now, and then work on it.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
OK, I agree. Let's freeze this for now, and then work on it.
OK - anybody else disagree (Jens, dmoore?!). Did somebody do some more testing? Results?
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
OK, I agree. Let's freeze this for now, and then work on it.
OK - anybody else disagree (Jens, dmoore?!). Did somebody do some more testing? Results?
No objections from my side.
I did not do any further tests, but I try to do it soon.