Community
Participate
Working Groups
See bug 206601 comment 3: > Looking at difference between v20071008 and v20071015 versions of project > org.eclipse.core.resources, I saw a change which may explain this small > regression: in File.getCharset(boolean) method now calls a new method > checkSynchronized() (also done in getContentDescription()). > > I confirm that the impacted JDT/Core tests go through this new method call > while reading the resources (parse to build the AST, parse to compile the > java file or parse to locate the matches during a search request...). > Move to Platform/Resources for further investigation I should open this bug instead of moving bug 206601 to Platform/Resources component. Having 2 bugs may allow to fix this issue both on Platform/Resources and JDT/Core components...
Szymon, I've put back bug 206601 in JDT/Core land to give us a chance to fix our regression independentely from Platform/Resources...
The summary of this regression is that the fix was essential in order to fix bug 186984, which could potentially cause data corruption if not fixed. The fix involved one extra call to the file system to obtain the file timestamp. If such a small change is causing a noticeable regression, perhaps the performance test is too fine-grained. I suggest marking WONTFIX.
(In reply to comment #2) > The summary of this regression is that the fix was essential in order to fix > bug 186984, which could potentially cause data corruption if not fixed. The > fix involved one extra call to the file system to obtain the file timestamp. If > such a small change is causing a noticeable regression, perhaps the performance > test is too fine-grained. I suggest marking WONTFIX. > I do not really agree with the "too fine-grained". IMO, this is the clearly the opposite: the tests include some other actions than "pure" JDT/Core tested ones (compilation, search request,...). In this peculiar case, the additional action is the read of the resources. One solution would have been to first read the files and only measure the time to compile them after, but we did not want to do this as this would not represent the time that user measured during the compilation. You can close this as WONTFIX, but I would suggest first to have a precise idea of the regression time cost. It was 3-4% for JDT/Core tests but do not forget that for those tests, not all the time is spent to read the files. So, 4% on our tests may represent a little bit more during to the files reading and may be noticeable by users in certain circumstances...
John, does Platform Resource have performance tests? How did they change with that fix?
I vote to leave the fix in i.e. close as WONTFIX.
I would NOT close it as WONTFIX. I think we want both the original fix, and a way to not lose performance. Slowing down an overall Java build by 3-4% is huge. As a comparison, this is how long it takes to run a complete flow analysis on the entire project... I suspect the overhead introduce by the charset fix is much more significant than the side-effects on JDT/Core measurements. Does the platform have performance tests for this ? Seems not to be the case... As a possible optimization, how frequent is it that each file has a custom encoding ? I would say is never is the case. So if the platform was doing some clever caching maybe it could save a lot.
Discussing with Frederic, most of our #getCharset() usecases are before actually reading the source of compilation units. Would it be possible to offer a helper method in the platform to combine the operations and thus save extra (independant) checks ?
> So if the platform was doing some clever caching maybe it could save a lot. We do cache this information in memory. The point of bug 186984 was that the cache state is invalid if the file has changed on disk since we last cached the information. I don't see how we can fix this without going to the disk to get the timestamp. On the other hand, maybe it's not worth handling the rare case of an file changing encoding out from under us if it has such a performance impact. An alternative would be to introduce a FORCE flag for this method. The compiler could use FORCE=true to indicate they don't care if the file is in sync, and the editor could use FORCE=false to indicate they prefer failing if the file is out of sync. The data corruption issue doesn't arise for the compiler because they are not modifying the source file - they are just reading it.
Wouldn't force set to true mean that we would always hit the disk to find the encoding ? How could this be any faster ? (However I like avoiding to fail) Alternatively, using a bogus (in memory) encoding (just the one in memory) could mean that we potentially would misread some files, and issue invalid diagnostics. So it would still result in unhappy situation for end user as he wouldn't immediately realize that resources were out of sync). The Java builder is reading resources content with force=true already. If there was a way to batch the charset read with contents, in force=true fashion, I think we'd be happy again.
I think that in case when we first check the charset and then read, we would prefer to have FORCE in getContents() method. Or maybe we can add versions with FORCE argument for all significant IFile methods, so people can decide when check if the resource is in sync. Makes sense?
I was suggesting that force be specified such that it always returns the cached value if available, and never check the sync state of the resource. Just to clarify one thing, I don't think a full build is showing a 3-4% regression - bug 206601 only mentions indexing/search and building type hierarchy.
Created attachment 82577 [details] Local run of FullSourceWorkspaceSearchTests without sync check in getCharset
Created attachment 82579 [details] Local run of FullSourceWorkspaceSearchTests with sync check in getCharset To put this in context, I'm seeing a 20ms regression on indexing of source for org.eclipse.jdt.core project, and no regression at all for testSearchField and testSearchType (or its hidden in the noise of deviation between runs).
Created attachment 82582 [details] Local run of FullSourceWorkspaceTypeHierarchyTests without sync check in getCharset
Created attachment 82583 [details] Local run of FullSourceWorkspaceTypeHierarchyTests without sync check in getCharset
Created attachment 82584 [details] Local run of FullSourceWorkspaceTypeHierarchyTests with sync check in getCharset And I don't see a measurable difference in performance of FullSourceWorkspaceTypeHierarchyTests with and without the resources fix. This is the only test that shows a consistent regression in build results page since the resources fix was introduced.
John, My personal experience says that it's infinitely hard to get solid conclusions from a perf test run only once or twice on non-dedicated box (Did you make your run on a Windows box or Linux one?). I opened this bug after having compared results from Releng database and our local Linux test box. We run all JDT/Core perf tests on a local *Linux* box dedicated to performance. For each I-build, we run all JDT/Core tests *20* times, remove the five best and the five worst results and compute the average using the 10 remaining ones. Doing that, we got a standard deviation below 1% for each test result which allow us to detect any variation over 1%... I agree that I didn't re-run all the JDT/Core tests 20 times without the Platform/Resources fix to verify that it work better. However, I run it 4 times on the testSearchType test and I retrieved the difference of 4% I noticed in my local results. At that time, I thought it was enough to prove that the regression came from this change. As you felt it differently, I've restarted a bunch of measures on the FullSourceWorkspaceTypeHierarchyTests#testPerfAllTypes. Here are my results using version v20071008 of org.eclipse.core.resources: Date Time PerfAllTypes Comment 11/12/2007 3:43 PM 5108 [1]err=2.6% 11/12/2007 3:45 PM 5171 [1]err=2.4% 11/12/2007 4:23 PM 5906 [1]err=2.2% 11/12/2007 4:28 PM 5858 [1]err=1.1% 11/12/2007 4:32 PM 5030 [1]err=2.6% 11/12/2007 4:38 PM 5812 [1]err=1.4% 11/12/2007 4:45 PM 5780 [1]err=1.1% 11/12/2007 4:51 PM 5267 [1]err=3.2% 11/12/2007 4:55 PM 5750 [1]err=1.7% 11/12/2007 5:01 PM 5628 [1]err=2.6% Average 5531 Stddev 6.25% And here are the results using version v20071026 of org.eclipse.core.resources: Date Time PerfAllTypes Comment 11/12/2007 1:41 PM 5622 [1]err=2.7% 11/12/2007 1:44 PM 5580 [1]err=2.8% 11/12/2007 1:47 PM 5997 [1]err=1.3% 11/12/2007 1:49 PM 5765 [1]err=2.5% 11/12/2007 1:51 PM 5716 [1]err=2.9% 11/12/2007 3:19 PM 5686 [1]err=2.5% 11/12/2007 3:22 PM 5795 [1]err=1.8% 11/12/2007 3:27 PM 5562 [1]err=2.9% 11/12/2007 3:35 PM 5485 [1]err=3.6% 11/12/2007 3:39 PM 5593 [1]err=3.1% Average 5680 Stddev 2.59% Difference 2.70% You can see that depending on the run you choose, you can easily conclude that it's faster with or without the fix. However, the average shows a 2.70% regression with the fix (v20071026) and tend to show that the fix might be slower. But you can finally argue that with standard deviations of 6.25% and 2.59% that does not prove anything... So the conclusion is that I need to run JDT/Core tests 20 times without the Platform/Releng fix, smooth numbers as we do for our local tests and see what would be the results. Then we could definitely conclude if the fix is really implied in the noticed regression or not. I'll be back as soon as I get these numbers...
Note, I completely believe there is a regression - the fix we made will certainly impact the performance of that method. I'm just not sure about how big the performance impact is in real user scenarios. Comment #6 suggests that the regression is as expensive as complete flow analysis of a project - if this is true we should definitely revert the resources change or find some other solution. On the other hand, when I looked at the test results, it looks like the impact is more like an extra 20ms on a search over a large project, which to me is an acceptable regression. I just want to find out if it's worth spending more time on this...
(In reply to comment #18) > Note, I completely believe there is a regression - the fix we made will > certainly impact the performance of that method. I'm just not sure about how > big the performance impact is in real user scenarios. Comment #6 suggests that > the regression is as expensive as complete flow analysis of a project - if this > is true we should definitely revert the resources change or find some other > solution. On the other hand, when I looked at the test results, it looks like > the impact is more like an extra 20ms on a search over a large project, which > to me is an acceptable regression. I just want to find out if it's worth > spending more time on this... > I agree that build tests are not so impacted than the other ones and I should have clarify this point earlier after that comment 6 was written... But note that 20ms on a test lasting around 800 ms makes a regression of 2.5%... Furthermore, it depends on build and the TypeHierarchy test already showed more than 5% regression in the fingerprint. Same for the search tests which showed for some builds a regression over 6% :-( If unfortunately there's no possible optimization on your side to balance the impact of the fix, then at least a specific org.eclipse.core test showing this regression might be written. So, we could refer to it in our fingerprint tests to let users know what happens and avoid them to have a bad feeling while looking at JDT/Core tests...
Here are the numbers I got running JDT/Core tests on our Linux performance test box using: 1) I20071107-1300 2) I20071107-1300 with org.eclipse.core.resources_3.3.100.v20071008.jar FullSourceWorkspaceASTTests - testDomAstCreationProjectJLS3(): 1) -2.02% 2) -0.26% => regression = 1.76% FullSourceWorkspaceBuildTests - testFullBuildDefault(): 1) -0.87% 2) +0.26% - testFullBuildProjectNoWarning(): 1) -1.39% 2) -0.15% - testFullBuildProjectDefault(): 1) -1.13% 2) +0.87% - testFullBuildProjectAllWarnings(): 1) +0.74% 2) +1.32% => regression between 0.58% and 2% FullSourceWorkspaceSearchTests - testIndexing(): 1) -3.16% 2) -0.76% - testIndexingOneProject(): 1) -3.70% 2) -1.25% - testSearchType(): 1) -2.28% 2) -0.41% - testSearchField(): 1) -1.88% 2) -0.17% - testSearchBinaryMethod(): 1) -2.27% 2) -0.16% - testSearchConstructor(): 1) -1.76% 2) -0.29% => regression between 1.47% and 2.45% FullSourceWorkspaceTypeHierarchyTests - testPerfAllTypes(): 1) -1.64% 2) -0.46% - testPerfClassWithPotentialSubinterfaces(): 1) -1.27% 2) -0.22% => regression between 1.05% and 1.18% Note that builds results show that these numbers would have been worst for a Windows box...
I appreciate the amount of effort that has gone into this analysis. At this point, our choices are between keeping the "slower but working" code or putting back the "fast but broken" code. As has been mentioned already, the cases under which the broken code actually fails are uncommon, so either option is possible. In an effort to get closure on this, I give +1 for leaving the slower but working code in. However, given the cross-team impact on performance, I would also like to give the other PMC leads the option of vetoing. Speak now: If someone -1s, then we will take the fix out and update the javadoc to indicate the limitation. If the fix is left in, and there really is no way to improve the performance elsewhere, and the fact that tests are showing up red is too frustrating, then mark the tests as invalid with a comment that puts the blame on the getCharset() method, and we'll re-run the baselines after R3.4 ships.
+1 for the slower but correct code.
On reflection, I actually prefer the "fast but broken" code. More specifically, I think we should revert the change to getCharset, and update the spec to reflect the original implementation - that it returns a cached value of the charset of the file (much like IResource#getLocalTimeStamp specifies that it returns a cached value of the local file's timestamp). Even with the file system synchronization check we added, there is always a risk that the charset of the file will change after the client obtained the charset, but before the client gets around to reading/writing the file. The only fail-safe way to avoid the data corruption that Dani mentions, is to perform the synchronization check atomically with the write to the file (calling setContents without the FORCE flag). For clients that are only reading files, I think we should allow the client to make the judgement call on whether they want to accept a cached value. For example, the biggest % performance regression is on search, and it seems quite reasonable that search may return some incorrect results in the rare case that the file has been externally modified to alter the character set, and the workspace has not since been refreshed. At least for me, I consider a 2% regression on *every* search for *all* users to be an unacceptable trade-off for handling such a rare case (a case that no user has ever reported, to my knowledge).
Reverting the fix and document it will not help existing clients of the API as they won't notice. If we decide to revert then we should either deprecate the existing method or put this into the migration guide. I could live with such a solution though my vote is for the correct code. Even with the correct code we could offer new API that behaves like the old one (as discussed in this bug).
-1 for the slower code. I fully agree with JohnA's comment 23. The new behavior is still not perfect in these corner situations, but always slower for good citizen. This is a no go.
Good enough. Please revert to the old code, then modify the javadoc to indicate the limitation, and add an entry to the migration guide, which says "We have always had this limitation, but we have now made that clear in the contract", or something like that.
I have backed out of the change to File.getCharset and updated the spec of IFile.getCharset to indicate that it returns a cached value of the encoding. Clients that want the synchronization check to be performed can call IFile.getContentDescription().getCharset() (or invoke IResource.refreshLocal on the file to be sure it's in sync).
I have added this section to the "recommended changes" section of the porting guide: The method <code>IFile.getCharset</code> returns a cached value for the file's encoding. If the file's encoding has been changed externally, and has not since been synchronized with the workspace using <code>IResource.refreshLocal</code>, it may return a stale result. Clients that call this method should revisit their usage to determine if a cached result is acceptable. While using a cached encoding has better performance, it could lead to data corruption if the wrong encoding is used to write a file.
Verified in I20071210-1800.
