Community
Participate
Working Groups
Since parent is effective final the result will never change. We got a big xtext Project where JavaElement.getJavaProject() is a hotspot. The same optimisation can be applied to getJavaModel() getAncestor() isAncestorOf() even though they are no known hotspots for us. Its cheep to cache.
Created attachment 285577 [details] Screnshot of VisualVM 2.0.5 during full build showing getJavaProject
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/176407
Did not cache: getAncestor(r) isAncestorOf(e) since they take an argument which is kinda overkill to cache if not a hotspot.
@Jörg : any performance data available showing before and after patch ?
> @Jörg : any performance data available showing before and after patch ? Hi Manoj, i compared too recent samples with and without patch. The total time used (according to sampling) was reduced from 968ms to 191ms. i.e. 5x faster.
Created attachment 285907 [details] sampling changes compared
Moving target to the next M1
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/176407 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e272418664889ea9c3e0b165831872cc96e6eb69
Fabrice, since you reviewed this, did you consider the extra memory the caching is going to cost us? Do you have the numbers for the same? Please share it here.
(In reply to Jörg Kubitz from comment #6) > Created attachment 285907 [details] > sampling changes compared Is the CPU figure given is average or actual? This comparison is not helping since the number of hits is not same (25 vs 111). Are we experiencing less number of hits to getJavaProject() after the patch? It will be lot easier if the snapshots themselves are attached rather than a screenshot. Can you please do that?
(In reply to Jay Arthanareeswaran from comment #10) > (In reply to Jörg Kubitz from comment #6) > > Created attachment 285907 [details] > > sampling changes compared > > Is the CPU figure given is average or actual? This comparison is not helping > since the number of hits is not same (25 vs 111). Are we experiencing less > number of hits to getJavaProject() after the patch? > > It will be lot easier if the snapshots themselves are attached rather than a > screenshot. Can you please do that? I doubt i am allowed to share stacktraces of proprietary closed source code involved. The figures are from *sampling* done with VisualVM. This works by taking stacktraces at a fixed rate. The number of stacktraces for a certain function are reported as "Hits". The more "Hits" the more time was spend on that function. Given the sampling frequency is high enough the time spend is approximately proportional to the number of "Hits". It was the very reason for this change to reduce the time. => It is the EXPECTED and WANTED behaviour to reduce the number of Hits. Please do not confuse "sampling" with "profiling". To ask about "average or actual" does not make any sense in context of sampling.
(In reply to Jörg Kubitz from comment #11) > I doubt i am allowed to share stacktraces of proprietary closed source code > involved. > The figures are from *sampling* done with VisualVM. This works by taking > stacktraces at a fixed rate. The number of stacktraces for a certain > function are reported as "Hits". The more "Hits" the more time was spend on > that function. Given the sampling frequency is high enough the time spend is > approximately proportional to the number of "Hits". It was the very reason > for this change to reduce the time. => It is the EXPECTED and WANTED > behaviour to reduce the number of Hits. > Please do not confuse "sampling" with "profiling". To ask about "average or > actual" does not make any sense in context of sampling. I see. Sorry I didn't notice you mentioning sampling earlier. And I am not that familiar with VisualVM interface. That's why a snapshot is much more useful than a trimmed screenshot none (other than you, i.e.) have any clue about. If you want to make changes like these that involves so many files and touching APIs, we can never be too careful. Also, do respond to comment #9 as well. I doubt Fabrice had time to think about it.
Reopening as we need to investigate comparator errors caused see bug 574089
(In reply to Sravan Kumar Lakkimsetti from comment #13) > Reopening as we need to investigate comparator errors caused see bug 574089 Sorry wrong its bug 574088
(In reply to Eclipse Genie from comment #8) > Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/176407 was > merged to [master]. @Fabrice: I had assigned Kalyan as an additional reviewer in the gerrit - You need to wait for his +1 before pushing this change. Please be mindful of this in the future. Thanks
(In reply to Jay Arthanareeswaran from comment #9) > did you consider the extra memory the caching is going to cost us? Do you > have the numbers for the same? Please share it here. I added 2 References per JavaElement which are at most (if not erased by compiler)) 2*32 Bit pointers if using 32 Bit JVM or 64 Bit JVM with compressed references. Without compressed references it would be 2*64 Bit on a 64 Bit JVM. I did not measure it because it is obvious by specification. Feel free to verify it with any usecase.
(In reply to Jörg Kubitz from comment #16) > I added 2 References per JavaElement which are at most (if not erased by > compiler)) 2*32 Bit pointers if using 32 Bit JVM or 64 Bit JVM with > compressed references. Without compressed references it would be 2*64 Bit on > a 64 Bit JVM. > I did not measure it because it is obvious by specification. > Feel free to verify it with any usecase. That much is obvious from your patch. What I want to know is, for the performance boost you are claiming (for your use case i.e.), what is the cost in terms of memory? So far, you have not shared any data that shows how many JavaElement objects were created. Correct me if I am wrong.
(In reply to Jay Arthanareeswaran from comment #17) The measurement is now 3 month old - i cannot reproduce exactly that anymore. Also i doubt the size of my workspace is relevant to you nor could you verify it. Nor is memory any issue on my setup. If i clean compile "org.eclipse.jdt.core" (something you could verify but does not involve xtext) i end up with: 27268 JavaElements created 194143 calls to getJavaProject() (i.e. ~ 7 calls per element) 24864 calls to getJavaModel() (i.e. ~ 1 call per element) 27268 JavaElements * 8 bytes = 0.2 MB
For curiosity: In our current setup we have 10,043,007 JavaModel instances 32,818,934 calls to getJavaProject() i.e. ~ 3 calls per instance during build The speedup for us comes at the expense of ~ 80 MB.
(In reply to Jörg Kubitz from comment #18) > (In reply to Jay Arthanareeswaran from comment #17) > > The measurement is now 3 month old - i cannot reproduce exactly that > anymore. Also i doubt the size of my workspace is relevant to you nor could > you verify it. Nor is memory any issue on my setup. I just wanted to see the trade-off. (In reply to Jörg Kubitz from comment #19) > For curiosity: In our current setup we have > 10,043,007 JavaModel instances > 32,818,934 calls to getJavaProject() i.e. ~ 3 calls per instance during > build > > The speedup for us comes at the expense of ~ 80 MB. That's the number I wanted to see. A memory demand of this magnitude can't be called negligible, IMO. In my full JDT workspace, a workspace open followed by a full build creates 364,098 elements and another clean build creates 180,006 Java element Objects. One can argue that these get GCed, but we have to make sure we don't push the default heap limit.
(In reply to Jay Arthanareeswaran from comment #20) > > The speedup for us comes at the expense of ~ 80 MB. > > That's the number I wanted to see. A memory demand of this magnitude can't > be called negligible, IMO. For our workspace - where we allocate hundreds of GB at the same time - it is just noise. > In my full JDT workspace, a workspace open followed by a full build creates > 364,098 elements and another clean build creates 180,006 Java element > Objects. One can argue that these get GCed, but we have to make sure we > don't push the default heap limit. Then please share the numbers how much it influenced your heap limit. It should be well below 2 MB. thats ok for me.
(In reply to Jörg Kubitz from comment #21) > For our workspace - where we allocate hundreds of GB at the same time - it > is just noise. > > Then please share the numbers how much it influenced your heap limit. It > should be well below 2 MB. thats ok for me. That's the thing. I don't have the liberty to say "that's okay for me" :), even though I run it with heap that's much larger than the default of 2GB and I didn't see any impact on the memory with your patch. But then I have to think about several others who may never touch their memory settings. For the record, this is not the first time somebody said, "memory is cheap these days". But as always, we have to think about all type of Eclipse users. So, we try to get the full picture before letting any performance fix in. Anyway, the change is in, and I suppose we can let this remain and see how it goes.
> That's the thing. I don't have the liberty to say "that's okay for me" :), > even though I run it with heap that's much larger than the default of 2GB > and I didn't see any impact on the memory with your patch. But then I have > to think about several others who may never touch their memory settings. For > the record, this is not the first time somebody said, "memory is cheap these > days". But as always, we have to think about all type of Eclipse users. So, > we try to get the full picture before letting any performance fix in. Sorry i did not know how much JDT worries about memory. We could probably shrink that memory by avoiding to cache JavaModel. I am not familiar with the details of that model hierarchy but isn't JavaModel always the parent of the (cached) JavaProject? (or even always the JavaModelManager.getJavaModelManager().getJavaModel() Singleton?)
I was wondering if we could add an option for people to decide if they want to choose performance over memory? So projects affording more memory can use this feature by selecting a preference?
(In reply to Sarika Sinha from comment #24) > I was wondering if we could add an option for people to decide if they want > to choose performance over memory? > > So projects affording more memory can use this feature by selecting a > preference? In general a nice idea. In this particular case impossible (or way too complicated) - even an empty reference eats the same amount inside the datastructure. Also introducing conditionals for the option might eat performance.
(In reply to Jörg Kubitz from comment #25) > (In reply to Sarika Sinha from comment #24) > > I was wondering if we could add an option for people to decide if they want > > to choose performance over memory? > > > > So projects affording more memory can use this feature by selecting a > > preference? > > In general a nice idea. In this particular case impossible (or way too > complicated) - even an empty reference eats the same amount inside the > datastructure. Also introducing conditionals for the option might eat > performance. Yes, it will be tricky to achieve here. We need 2 different set of implementations itself. And one of the sets should be loaded based on the selection.
(In reply to Sarika Sinha from comment #26) > Yes, it will be tricky to achieve here. We need 2 different set of > implementations itself. Having 2 different set of implementations would render the types not effective final which could introduce conditional jumps by the hotspot compiler. i.e. performance loss.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/181744 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=c78718e58b70762188c0e3261582ce37c3b769eb
Verified for 4.21 M1.
