Community
Participate
Working Groups
There is more than 100% performance degradation of this test on Windows. It looks good on Linux. This is first seen from I20110222-0800.
I will look into this.
Fix for bug 336046 is causing this performance regression. Jay, any comments/ideas?
Patch for bug 336046 includes check for existence of the source attachment. The test case has one project org.eclipse.tomcat which has the source attachment as D:/tomcat4.1.30src and my machine D drive is not ready and a call to File#exists() for this file seems to take considerable amount of time. Though the regression is because of this particular test case, I think the number of calls to this function could be limited, which needs to be explored.
(In reply to comment #3) > Though the regression is because of this particular test case, I think the > number of calls to this function could be limited, which needs to be explored. There doesn't seem to be a way to reduce the number of calls. In this particular test case, it can be reduced but not generally. In this particular case, org.eclipse.tomcat's class path has references to 22 libraries whose sources lies in D:\. Thus they are 22 calls and hence keeping a list of non-existing directories could help in this case, but it may not generically. Actually there are 22 calls to ExternalFoldersManager#isExternalFolderPath(), but this function first calls File#isFile() and then calls File#exists(). Both of them are taking lot of time because of the unmapped drive. exists() probably can be called before isFile(). Doing this reduces the regression to 50%, but the regression still exists. Olivier/Srikanth, What do you think we should do for this bug? Option 1: Live with the regression, as the problem is only for unmapped drives. Option 2: Change the order of isFile() and isExists(). Option 3: Change the test case: Change the source attachment in the classpath of our test plugin to C:\. I am inclined for Option 2 and Option 3. What is your take?
If the test case is pointing to a drive that doesn't exist, then yes, the test case should be fixed. Now we could investigate if there is a way to cache the values that don't map so that we don't need to test isFile() and exists() many times for the same path.
(In reply to comment #5) > If the test case is pointing to a drive that doesn't exist, then yes, the test > case should be fixed. Just some more data to make the decision - The testcase here is a set of Eclipse 3.5 plugins and one of the plugin is having a bad source attachment directory. It is likely that customer's environment also could have such plugins. > Now we could investigate if there is a way to cache the values that don't map > so that we don't need to test isFile() and exists() many times for the same > path. Generally, these functions are called once for a classpath entry. Here, as many libraries are pointing to the same directory, there are many calls. May be we could probably keep this in the memory until we finish the resolveClassPath(). I will work on it. Thanks.
Satyam, are you saying that the D drive was slow to read because it had spun down and needed time to spin back up? If so, that seems like it could be fixed by simply making sure the test case reads something from the D drive before timing begins. But if you are saying that the D drive does not exist on your machine, and therefore the test is slow because it has to wait for the read to time out, that seems like an error condition. Rather than try to make local optimizations to work around file system problems, in an error condition, I would suggest that the real solution here is to do a better job of reporting the error. That is, we need to clearly inform the user that we were unable to read from the expected location (and the error message needs to tell them what that location was). There are probably other consequences beyond just the slowdown, in this situation.
(In reply to comment #7) The D drive doesn't exist and here the problem is for the source attachments of a library. It is not really important but good to have and hence if the drive doesn't exist, it is no harm in ignoring it. However, File#exists() takes lot of time for a unmapped drive. Hence, the question here could be do we want to cache that info.
Created attachment 190629 [details] Proposed patch I have cached the information of the drives while parsing the classpath for a given project. I didn't want to cache it for the complete Eclipse session, because it is likely possibly that the drive is mapped after Eclipse comes up and we will loose that information. There is about 10% regression with this patch. By the way, without the patch, regression is 100%.
Jay, Can you please review the changes?
+1 for the patch.
Thanks Jay, Released the patch on HEAD
Verified for 3.7M6.
