Community
Participate
Working Groups
Resource loading is a significant part of the total build time of a project. For big projects it is beneficial to parallelize this process. I've attached a patch, it's made against the current head and it should pass all the existing tests for the builder. Resource loading happens twice in the builder. Once for building the global index and once for linking and validation. (Resource loading is parsing, building the node - & emf model) While building the global index, resource loading completely dominates the execution. The parallel resource loader is configured to create one thread for each core, the speedup on a 4 core machine is ~3. To optimize the thread utilisation, big resources are processed first. In the linking and validation phase, resource loading is a much smaller part. Here one thread will be used for resource loading and one thread (the main thread) is used for linking and validation.
Created attachment 203896 [details] parallel resource loading
Created attachment 203897 [details] fixed patch Fixed a small oversight in SharedModule
Scheduled for review. Knut, could you give it a try?
I will take a look at it.
I had trouble applying the patch. Is it possible that your clone was not updated before creating the patch?
Created attachment 203919 [details] Converted all line endings to unix
Some comments on the code: 0. First I'd like to say that I think the code is overall very clean! 1. TimeUnit.MINUTES was AFAIK added with Java 1.6. So we should probably stick to e.g. TimeUnit.SECONDS. 2. I think it would make sense to add String constants to e.g. ClusteringBuilderState for the two named loaders it uses. 3. In ClusteringBuilderState#writeNewResourceDescriptions() the loaded resource is never added to the local ResourceSet. I think we're missing a "resourceSet.getResources().add(resource);". 4. The ResourceLoadJob should probably deal with the InterruptedException somehow (e.g. requeue it) as the resource otherwise gets lost. Currently it silently ignores the exception. Same for ParallelLoadOperation#next(). 5. The ParallelResourceLoader should maybe also have something like the SerialResourceLoader's #checkStopped(). 6. Wouldn't it be better to call executor.shutdownNow() in #stop() (as opposed to shutdown())? I am thinking about cases when the builder fails with an exception. 7. Some Javadoc on the interfaces would be nice :-) 8. I don't think that we need to add anything to the SharedStateModule. At least not the named bindings only used by the builder. Other things I didn't understand or would like to discuss: A. Not sure I understand the reason for first processing large resources in ParallelResourceLoader. I noticed this because in our product we try to work with a fixed build order (sometimes computing the names or user data of exported objects requires references to be resolved; yuck!). Also I think that depending on what's behind the URI the createInputStream() could be an expensive operation. But if there are good reasons for this ordering, it would of course be easy for me to create my own implementation which doesn't reorder the URIs. B. As I understand the ParallelResourceLoader it will chug ahead loading resources until the queue is empty. Depending on how many threads are running it may be done far before the builder gets to the last resource. Our application is quite memory sensitive, so for us it would make more sense if the ParallelResourceLoader simply makes sure the queue always contains at least one more element. In our product the second build phase is actually faster with the SerialResourceLoader than the ParallelResourceLoader(1). C. Is what I describe in B maybe also why you had problems with parallel loading during the second build phase? Or what problems did you have? D. I think it would be interesting if the ResourceSet could during demand loading ask the resource loader if it already has the resource it wants loaded. But that would require the API of IResourceLoader to change as ResourceSetImpl internally works with demandCreateResource(URI) and demandLoadHelper(Resource). Any other thoughts on this?
Created attachment 203956 [details] updated patch with lazy parallel loading Another comment: Copyright comments would probably need to be changed. I've attached an updated patch which implements the lazy parallel loading strategy described under item B of comment #7. I made good results with this when using 4-way parallel loading for both loaders (around 20% lower build time). Please let me know how this works for you. Also note that the builder will discard the resource from the loader in case it already has the resource in its resource set.
(In reply to comment #0) > Resource loading happens twice in the builder. Once for building the global > index and once for linking and validation. (Resource loading is parsing, > building the node - & emf model) I don't think we need to clear the ResourceSet If no clustering happens, do we?
> Some comments on the code: > 1. TimeUnit.MINUTES was AFAIK added with Java 1.6. So we should probably stick > to e.g. TimeUnit.SECONDS. ok > 2. I think it would make sense to add String constants to e.g. > ClusteringBuilderState for the two named loaders it uses. ok > 3. In ClusteringBuilderState#writeNewResourceDescriptions() the loaded resource > is never added to the local ResourceSet. I think we're missing a > "resourceSet.getResources().add(resource);". I don't think it is necessary. > 4. The ResourceLoadJob should probably deal with the InterruptedException > somehow (e.g. requeue it) as the resource otherwise gets lost. Currently it > silently ignores the exception. Same for ParallelLoadOperation#next(). I'm not sure about this and I've spend a lot of time trying to figure this out, without coming up with a good answer. I think that queuing the exception is wrong, if one of the worker threads gets an interrupted exception we should interrupt the main thread. I've been reading http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html, it seems that if I run "Thread.currentThread().interrupt();" the ThreadPoolExecutor should detect it and take care of it. > 5. The ParallelResourceLoader should maybe also have something like the > SerialResourceLoader's #checkStopped(). > 6. Wouldn't it be better to call executor.shutdownNow() in #stop() (as opposed > to shutdown())? I am thinking about cases when the builder fails with an > exception. I've changed the resourceloader a bit. A loadoperation can now load only once, so #stop() becomes unnecessary. I've also removed the unused methods on that interface. > 7. Some Javadoc on the interfaces would be nice :-) ok > 8. I don't think that we need to add anything to the SharedStateModule. At least not the named bindings only used by the builder. I added it to make all the tests pass. (Specifically to make PersistableResourceDescriptionsTest pass) Maybe I should change the test? I'm not sure what the purpose is of SharedStateModule. Is there a way to have a default implementation for a named binding? > Other things I didn't understand or would like to discuss: > A. Not sure I understand the reason for first processing large resources in > ParallelResourceLoader. I noticed this because in our product we try to work > with a fixed build order (sometimes computing the names or user data of > exported objects requires references to be resolved; yuck!). Also I think that > depending on what's behind the URI the createInputStream() could be an > expensive operation. But if there are good reasons for this ordering, it would > of course be easy for me to create my own implementation which doesn't reorder > the URIs. At least for our workloads, typically we have one resource that dominates the resource loading. One 9 MB file and 150+ <100 kB files, if the big resource is processed last the workload distribution accross the threads is bad. Fixing the order improved the scaling from 2.5 to 3 on a 4 core machine. Processing order is important during the linking phase, but in my implementation I don't change the loadorder during in this phase. My method to guess the filesize is a (very ugly) hack but I can assure you it is always cheap. > B. As I understand the ParallelResourceLoader it will chug ahead loading > resources until the queue is empty. Depending on how many threads are running > it may be done far before the builder gets to the last resource. Our > application is quite memory sensitive, so for us it would make more sense if > the ParallelResourceLoader simply makes sure the queue always contains at least > one more element. In our product the second build phase is actually faster with > the SerialResourceLoader than the ParallelResourceLoader(1). This is not the case, the ParallelResourceLoader dumps its results in a bounded buffer and the consuming thread takes resources from that buffer. If the buffer is full, a resource loading thread blocks until a place is available. So the amount of resources in flight is the number of loading threads + the number of places in the buffer. In my implementation, during the linking phase the buffer has one slot and there is only one resource loading thread. I think what you are asking for is an implementation without buffering. I've implemented that in the updated version. > C. Is what I describe in B maybe also why you had problems with parallel > loading during the second build phase? Or what problems did you have? I had problems because I was trying to replace the queuing structure and because I was trying to very gracefully cancel already scheduled resource loads (instead of just cancelling the whole Executor & starting a new one). The current design is a lot less complicated than the old one. > D. I think it would be interesting if the ResourceSet could during demand > loading ask the resource loader if it already has the resource it wants loaded. > But that would require the API of IResourceLoader to change as ResourceSetImpl > internally works with demandCreateResource(URI) and demandLoadHelper(Resource). > Any other thoughts on this? I think this could be done but I don't think we will gain much and the synchronization could be tricky. Before we do this I would like to measure how often we load too many resources. > I've attached an updated patch which implements the lazy parallel loading > strategy described under item B of comment #7. I made good results with this > when using 4-way parallel loading for both loaders (around 20% lower build > time). Please let me know how this works for you. I've taken a quick look, did you see 4 cores running? I think your implementation can only load resources one by one. > Also note that the builder will discard the resource from the loader in case it > already has the resource in its resource set. Nice, did you measure how much this helps? I'm still testing my updated version, your feedback has been very helpful, I'll try to post a new patch in the afternoon.
(In reply to comment #10) > > 3. In ClusteringBuilderState#writeNewResourceDescriptions() the loaded resource > > is never added to the local ResourceSet. I think we're missing a > > "resourceSet.getResources().add(resource);". > I don't think it is necessary. I think it would be more consistent. Also I noticed this because we unfortunately have some cross reference resolution going on during the first build phase, in which case it would make sense to use the builder's resource set. Further, if we decide not to add resources to the builder's resource set, then we should also change the exception handling in writeNewResourceDescriptions() and not clear the resource set at the end. > > 4. The ResourceLoadJob should probably deal with the InterruptedException > > somehow (e.g. requeue it) as the resource otherwise gets lost. Currently it > > silently ignores the exception. Same for ParallelLoadOperation#next(). > I'm not sure about this and I've spend a lot of time trying to figure this out, > without coming up with a good answer. > I think that queuing the exception is wrong, if one of the worker threads gets > an interrupted exception we should interrupt the main thread. > I've been reading > http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html, it seems > that if I run "Thread.currentThread().interrupt();" the ThreadPoolExecutor > should detect it and take care of it. OK. > > 8. I don't think that we need to add anything to the SharedStateModule. At > least not the named bindings only used by the builder. > I added it to make all the tests pass. (Specifically to make > PersistableResourceDescriptionsTest pass) > Maybe I should change the test? I'm not sure what the purpose is of > SharedStateModule. > Is there a way to have a default implementation for a named binding? The SharedStateModule defines bindings common to all injectors created by the UI plug-in of Xtext languages. Most of these bindings will delegate to the injector created for the org.eclipse.xtext.ui.shared plug-in, which mostly contains singletons representing shared state (e.g. the IBuilderState). For the PersistableResourceDescriptionsTest I think it would be enough to change the access to: builderState = (ClusteringBuilderState) builderInjector.getInstance(IBuilderState.class); instead of: builderState = builderInjector.getInstance(ClusteringBuilderState.class); > > Other things I didn't understand or would like to discuss: > > A. Not sure I understand the reason for first processing large resources in > > ParallelResourceLoader. I noticed this because in our product we try to work > > with a fixed build order (sometimes computing the names or user data of > > exported objects requires references to be resolved; yuck!). Also I think that > > depending on what's behind the URI the createInputStream() could be an > > expensive operation. But if there are good reasons for this ordering, it would > > of course be easy for me to create my own implementation which doesn't reorder > > the URIs. > At least for our workloads, typically we have one resource that dominates the > resource loading. > One 9 MB file and 150+ <100 kB files, if the big resource is processed last the > workload distribution accross the threads is bad. Fixing the order improved the > scaling from 2.5 to 3 on a 4 core machine. > Processing order is important during the linking phase, but in my > implementation I don't change the loadorder during in this phase. > My method to guess the filesize is a (very ugly) hack but I can assure you it > is always cheap. I suppose this could be decoupled if the builder would do the sorting before handing the URIs to the resource loader. That's what we have done so far by injecting something like an IBuildOrderer into our custom ClusteringBuilderState implementation. What do you think? > > B. As I understand the ParallelResourceLoader it will chug ahead loading > > resources until the queue is empty. Depending on how many threads are running > > it may be done far before the builder gets to the last resource. Our > > application is quite memory sensitive, so for us it would make more sense if > > the ParallelResourceLoader simply makes sure the queue always contains at least > > one more element. In our product the second build phase is actually faster with > > the SerialResourceLoader than the ParallelResourceLoader(1). > This is not the case, the ParallelResourceLoader dumps its results in a bounded > buffer and the consuming thread takes resources from that buffer. If the buffer > is full, a resource loading thread blocks until a place is available. So the > amount of resources in flight is the number of loading threads + the number of > places in the buffer. In my implementation, during the linking phase the buffer > has one slot and there is only one resource loading thread. > I think what you are asking for is an implementation without buffering. I've > implemented that in the updated version. > > > C. Is what I describe in B maybe also why you had problems with parallel > > loading during the second build phase? Or what problems did you have? > I had problems because I was trying to replace the queuing structure and > because I was trying to very gracefully cancel already scheduled resource loads > (instead of just cancelling the whole Executor & starting a new one). The > current design is a lot less complicated than the old one. > > > D. I think it would be interesting if the ResourceSet could during demand > > loading ask the resource loader if it already has the resource it wants loaded. > > But that would require the API of IResourceLoader to change as ResourceSetImpl > > internally works with demandCreateResource(URI) and demandLoadHelper(Resource). > > Any other thoughts on this? > I think this could be done but I don't think we will gain much and the > synchronization could be tricky. Before we do this I would like to measure how > often we load too many resources. Agreed. > > I've attached an updated patch which implements the lazy parallel loading > > strategy described under item B of comment #7. I made good results with this > > when using 4-way parallel loading for both loaders (around 20% lower build > > time). Please let me know how this works for you. > I've taken a quick look, did you see 4 cores running? I think your > implementation can only load resources one by one. I think I didn't study your initial patch carefully enough. But I think I now know why it didn't perform very well for me: The machine I test on has 24 cores which means that there will typically be 48 resources loaded in memory by the resource loader. That requires quite a bit of extra memory. I think we should instead use different defaults. I assume that a fixed value (e.g. 3 or 4) would make sense as the build may otherwise even be slower on machines with many cores. So I think the main difference between your implementation and my rather quick hack is that with your implementation there would at most be queueSize + nThreads resources loaded into memory, while with my implementation this would be limited to queueSize. Although also with my implementation it could happen that the queue is temporarily empty while there are multiple threads busy loading. I would still be interested in how my implementation performs in your environment. And I will also run some tests again with your implementation and a lower parallelization (e.g. 4). > > Also note that the builder will discard the resource from the loader in case it > > already has the resource in its resource set. > Nice, did you measure how much this helps? No, I didn't. Probably not much. But it makes sure that I can't have the same resource loaded multiple times in the same resource set. > I'm still testing my updated version, your feedback has been very helpful, I'll > try to post a new patch in the afternoon. Looking forward to it!
(In reply to comment #9) > I don't think we need to clear the ResourceSet If no clustering happens, do we? I suppose you're referring to the clear at the end of the first build phase. Correct? Even if the resource set was cleared a few times during the first build phase, we wouldn't have to clear it at the end. I suppose that for the second build phase one would then start by processing the resources which are still loaded. But once the clustering policy kicks in the resource set would have to be cleared. But if it is just a small incremental build, chances are quite good that this won't happen and then every resource was just loaded once.
After your latest dynamic clustering contributions clustering isn't used in most cases. So not clearing the resource set in these cases seems to be a very good idea.
(In reply to comment #13) > After your latest dynamic clustering contributions clustering isn't used in > most cases. So not clearing the resource set in these cases seems to be a very > good idea. For this I think it would also make sense to use the same clustering approach inside writeNewResourceDescriptions() (i.e. for the first build phase). This has certainly proven to perform better in our product. I've run some more performance tests on a 24 core machine. With Lieven's currently attached implementation the performance increases up to a parallelization degree of 4 (for both resource loaders). Increasing the parallelization degree doesn't make any measurable difference. With that our build time is down to around 7 minutes.
Created attachment 204013 [details] Updated patch So this is the updated patch. I've rerun most of my measurements, my small test project is ~40% faster with parallel loading (keep in mind that I have to load every resource about 3 times, once for the global index, once for linking and once in the builder participant) I found that the buffer in between the producing & consuming thread doesn't help much (< 5%) so by default I've disabled buffering to preserve memory. I've set the number of resourceloading threads for the linker to 2, that seems to give an extra boost. I've made it possible to enable/disable sorting by filesize. I added some Javadoc on IResourceLoader & made the @Named Strings constant. I wasn't able to fix the injectionproblem with SharedStateModule and PersistableResourceDescriptionsTest.
(In reply to comment #15) > I found that the buffer in between the producing & consuming thread doesn't > help much (< 5%) so by default I've disabled buffering to preserve memory. I've made the same observations. > I've set the number of resourceloading threads for the linker to 2, that seems > to give an extra boost. I achieved better performance with a higher parallelization degree for the linking phase. 4 and 4 seemed best for us. But this is of course something power users can tweak using an overriding Guice module. > I've made it possible to enable/disable sorting by filesize. > I added some Javadoc on IResourceLoader & made the @Named Strings constant. Excellent. > I wasn't able to fix the injectionproblem with SharedStateModule and > PersistableResourceDescriptionsTest. I can take care of that. I think it should only be a matter of making the test use the SharedModule directly, where the bindings are defined. A questions regarding the latest change: Is it on purpose that ResourceLoaderProviders#getParallelLoader() uses Math#max() and not Math#min() ? I have three more API changes I'd like to discuss: 1. What if we'd simplify LoadOperation#next() to return just a Resource instead of a Pair<URI, Resource>? The client can easily obtain the URI from the resource. 2. I would like to implement a custom resource loader which dynamically (with each invocation of next()) decides which resources should be made available next with the goal to minimize the number of resources the builder loads itself (as part of the linking). The algorithm behind this would ideally also respect what resources are already loaded into the resource set. So I was wondering what you think about changing the API of e.g. IResourceLoader#create(URIConverter) to IResourceLoader#create(ResourceSet) or LoadOperation#load(Collection<URI>) to LoadOperation#load(ResourceSet, Collection<URI>) ? 3. What about defining an extra interface like IResourceLoader$Sorter with a method sort(Collection<URI>) : Collection<URI> which could be optionally injected into the IResourceLoader implementation? Could be used to implement a fixed ordering or a ordering by file size.
(In reply to comment #16) > (In reply to comment #15) > > I found that the buffer in between the producing & consuming thread doesn't > > help much (< 5%) so by default I've disabled buffering to preserve memory. > > I've made the same observations. > > > I've set the number of resourceloading threads for the linker to 2, that seems > > to give an extra boost. > > I achieved better performance with a higher parallelization degree for the > linking phase. 4 and 4 seemed best for us. But this is of course something > power users can tweak using an overriding Guice module. > > > I've made it possible to enable/disable sorting by filesize. > > I added some Javadoc on IResourceLoader & made the @Named Strings constant. > > Excellent. > > > I wasn't able to fix the injectionproblem with SharedStateModule and > > PersistableResourceDescriptionsTest. > > I can take care of that. I think it should only be a matter of making the test > use the SharedModule directly, where the bindings are defined. > > A questions regarding the latest change: Is it on purpose that > ResourceLoaderProviders#getParallelLoader() uses Math#max() and not Math#min() > ? That should be Math#min(). > > I have three more API changes I'd like to discuss: > > 1. What if we'd simplify LoadOperation#next() to return just a Resource instead > of a Pair<URI, Resource>? The client can easily obtain the URI from the > resource. > It would make the API cleaner, I'll do it. > 2. I would like to implement a custom resource loader which dynamically (with > each invocation of next()) decides which resources should be made available > next with the goal to minimize the number of resources the builder loads itself > (as part of the linking). The algorithm behind this would ideally also respect > what resources are already loaded into the resource set. So I was wondering > what you think about changing the API of e.g. > IResourceLoader#create(URIConverter) to IResourceLoader#create(ResourceSet) or > LoadOperation#load(Collection<URI>) to LoadOperation#load(ResourceSet, > Collection<URI>) ? I've already done that. In my latest version I can skip resources that are already loaded into the ResourceSet of the main thread. (as you suggested in point D of comment #7) Using this & Sven's suggestion (comment #9) I've seen some moderate gains for small builds. This change also reduces the number of IResourceLoader#create methods. > 3. What about defining an extra interface like IResourceLoader$Sorter with a > method sort(Collection<URI>) : Collection<URI> which could be optionally > injected into the IResourceLoader implementation? Could be used to implement a > fixed ordering or a ordering by file size. Ok, making this pluggable would be nice. I'll look into it. About ordering, you can guarantee the order that resources to into the resource loader but that doesn't guarantee the order that they are returned with next() (at least with my implementation). This is something you will have to implement. I'll have a new patch ready asap.
Hi Lieven, I'm really excited about the progress that you made so far. Would it be possible to something like #canBuildInParallel to IResourceServiceProvider which returns false by default. I'm a little bit concerned about backwards compatibility for languages that resolve cross references by accident in the first build phase. All resources that belong to such a language should be loaded sequentially while the others may be loaded in parallel before the sequential load kicks in. Does that make sense?
(In reply to comment #18) > I'm really excited about the progress that you made so far. > Would it be possible to something like #canBuildInParallel to > IResourceServiceProvider which returns false by default. I'm a little bit > concerned about backwards compatibility for languages that resolve cross > references by accident in the first build phase. All resources that belong to > such a language should be loaded sequentially while the others may be loaded in > parallel before the sequential load kicks in. Does that make sense? How does loading sequentially make a difference? Are you thinking of languages which don't use the LazyLinker and thus do the cross reference resolution in the load phase? We have quite a few languages which resolve references in the first build phase, but not until the exported objects are computed. And then it doesn't matter whether the resource was loaded sequentially or in parallel. What matters is the order in which they are processed by the builder. I.e. have the resources it tries to resolve already been processed? Our builder deals with this by ordering the resources and if this should fail the builder detects this and requeues the resources. If worse comes to worse (the references cannot be resolved) an error is logged and the resource is skipped. Do you suggest that we also need something like this in Xtext's ClusteringBuilderState?
Created attachment 204100 [details] updated patch - Fixed max -> min - LoadOperation#next() returns Resource - Not clearing ResourceSet after building the global index - Prevent loading of resources that were already loaded (it's a small gain, for incremental builds) The synchronization gets trickier and reduced the scaling to 2.6 on my 4 core machine. I'm leaving it in because I think it will reduce the memory pressure. - Added pluggable sorting - Simplified SerialResourceLoader I don't have much time left this week to work on this, but I think we're close to finishing this enhancement.
Created attachment 204146 [details] Fixed sorting bug
Created attachment 204148 [details] updated patch The attached patch contains the following changes: - supports sorting in the SerialResourceLoader - removed bindings from SharedStateModule - use same clustering approach for first build phase - added required @since tags - default parallel loader will now at least have two threads (I think that should even be beneficial on a single core machine) - temporarily using SynchronizedXtextResourceSet (see below) Despite the synchronization I got a lot of ConcurrentModificationExceptions with the latest changes. Let me know if you need a sample workspace to reproduce this. The reason is probably that the builder will also modify the resource set in non synchronized blocks (e.g. when resources are dynamically loaded due to cross reference resolution). This would probably be fixed by using a SynchronizedXtextResourceSet as in the patch. But I think that will significantly lower the throughput, so I think we should probably revert that last change. As an alternative we could maybe create an XtextResourceSet implementation which itself includes the resource loader. But that may require some more thinking (and more work).
Created attachment 204160 [details] removed optimization (In reply to comment #22) > Created attachment 204148 [details] > updated patch > > The attached patch contains the following changes: > - supports sorting in the SerialResourceLoader > - removed bindings from SharedStateModule > - use same clustering approach for first build phase > - added required @since tags > - default parallel loader will now at least have two threads (I think that > should even be beneficial on a single core machine) > - temporarily using SynchronizedXtextResourceSet (see below) > > Despite the synchronization I got a lot of ConcurrentModificationExceptions > with the latest changes. Let me know if you need a sample workspace to > reproduce this. The reason is probably that the builder will also modify the > resource set in non synchronized blocks (e.g. when resources are dynamically > loaded due to cross reference resolution). This would probably be fixed by > using a SynchronizedXtextResourceSet as in the patch. But I think that will > significantly lower the throughput, so I think we should probably revert that > last change. > > As an alternative we could maybe create an XtextResourceSet implementation > which itself includes the resource loader. But that may require some more > thinking (and more work). I think it would be better to handle that optimization in a separate ticket and we should focus on finishing this one. I've removed the optimization.
(In reply to comment #19) > How does loading sequentially make a difference? Are you thinking of languages > which don't use the LazyLinker and thus do the cross reference resolution in > the load phase? I just want to make sure that we do not break existing languages with a multi threaded loading which is inherently hard to debug et al. Nevertheless I see you point that it does not really make a difference if this is done in the serial mode or in parallel mode. The only thing that puzzles me is the fact that you may end up with resources that have been reloaded twice due to unintentionally resolved cross references. #addResource will add the loaded resource to the builder's resource set, if it is absent. This resource may be partially resolved (by accident) and point to another resource that was contained in the loader's resource set. This raises the question how the partially resolved resource can be resolved completely in a consistent manner. If everything was loaded in the same resource set, this wouldn't be a problem at all. Knut, since you face the situation that you have to resolve some references in the first build cycle, could you please gather some statistics how often the loaded resource is rejected in #addResource if you load in parallel? (In reply to comment #23) > I think it would be better to handle that optimization in a separate ticket and > we should focus on finishing this one. > +1
(In reply to comment #24) > I just want to make sure that we do not break existing languages with a multi > threaded loading which is inherently hard to debug et al. Nevertheless I see > you point that it does not really make a difference if this is done in the > serial mode or in parallel mode. The only thing that puzzles me is the fact > that you may end up with resources that have been reloaded twice due to > unintentionally resolved cross references. AFAICT, the only thing that could cause problems is if a resource attempts to resolve references as part of the Resource#load() call. An XtextResource would probably only do this if it uses a custom non LazyLinker or if it overrides AbstractCleaningLinker#afterModelLinked() to do something unusual. Of course attempts to resolve references against the index cannot be expected to succeed at this point. But it could be a problem if additional resources are loaded into the resource set. I think we should be able to detect this kind of problem by checking whether the loader's resource set contains any additional resources after loading the requested resource. An idea how to deal with this: The builder (client) could after the load operation is done (hasNext() returns "false") query these somehow (e.g. #getUnloadableResources() : Collection<URI>) and decide what to do with them (the builder could try to load them on its own). The alternative is of course the flag you propose. But that would mean that the builder would first have to check if any of the URIs cannot be loaded in parallel and it would then have to either (a) load them all itself as in the current implementation or (b) load some of them itself and some using the resource loader. If the default for this flag is to be parallel=true we'd at least have to build some kind of detection (e.g. as proposed). > #addResource will add the loaded resource to the builder's resource set, if it > is absent. This resource may be partially resolved (by accident) and point to > another resource that was contained in the loader's resource set. This raises > the question how the partially resolved resource can be resolved completely in > a consistent manner. If everything was loaded in the same resource set, this > wouldn't be a problem at all. > > Knut, since you face the situation that you have to resolve some references in > the first build cycle, could you please gather some statistics how often the > loaded resource is rejected in #addResource if you load in parallel? I probably wasn't clear enough. We do have resources which require reference resolution during the first build phase. But not as part of Resource#doLoad(). It's part of the IResourceDescription#getExportedObjects() call. And this isn't done until the resource has been added to the builder's resource set. To answer your question: We don't have any resources being rejected in the first phase. In the second build phase the situation is different: Here we call EcoreUtil2#resolveLazyCrossReferences() which may end up loading resources on demand (into the builder's resource set) which haven't been processed yet by the builder. Thus I suggested that the builder first checks if it already has the resource loaded before it adds the resource obtained from the loader. Hope that was clearer :-) > (In reply to comment #23) > > I think it would be better to handle that optimization in a separate ticket and > > we should focus on finishing this one. > > > > +1 +1. But the optimization which I think should be removed is the method AbstractResourceLoader#loadResource() which also tries to load resources from the builder's resource set. There are probably too many patches in circulation now, but I will add one last patch which covers my point of view.
Created attachment 204171 [details] the right patch I uploaded the wrong file, this one has the optimization removed.
(In reply to comment #25) > (In reply to comment #24) > AFAICT, the only thing that could cause problems is if a resource attempts to > resolve references as part of the Resource#load() call. An XtextResource would > probably only do this if it uses a custom non LazyLinker or if it overrides > AbstractCleaningLinker#afterModelLinked() to do something unusual. Of course > attempts to resolve references against the index cannot be expected to succeed > at this point. But it could be a problem if additional resources are loaded > into the resource set. This will happen as soon as you have opposite references or used #resolveProxies == false for the cross reference. This may especially happen with existing EPackages such as Ecore.ecore. Please see LazyLinker.installQueuedLinks(Multimap<Setting, INode>). > I think we should be able to detect this kind of problem by checking whether > the loader's resource set contains any additional resources after loading the > requested resource. An idea how to deal with this: The builder (client) could > after the load operation is done (hasNext() returns "false") query these > somehow (e.g. #getUnloadableResources() : Collection<URI>) and decide what to > do with them (the builder could try to load them on its own). I'm not sure that I understood your suggestion. How would that help to fix the broken (resolved) links in the loaded resource itself? Please note that such languages are unlikely to use an unloader for their resource. > > The alternative is of course the flag you propose. But that would mean that the > builder would first have to check if any of the URIs cannot be loaded in > parallel and it would then have to either (a) load them all itself as in the > current implementation or (b) load some of them itself and some using the > resource loader. If the default for this flag is to be parallel=true we'd at > least have to build some kind of detection (e.g. as proposed). I was thinking about parallel=false as the default. All uris that cannot be processed in parallel could be postponed (queued) to the end and processed after the parallel loading happend. > I probably wasn't clear enough. We do have resources which require reference > resolution during the first build phase. But not as part of Resource#doLoad(). > It's part of the IResourceDescription#getExportedObjects() call. And this isn't > done until the resource has been added to the builder's resource set. > > To answer your question: We don't have any resources being rejected in the > first phase. That's good :) > > In the second build phase the situation is different: Here we call > EcoreUtil2#resolveLazyCrossReferences() which may end up loading resources on > demand (into the builder's resource set) which haven't been processed yet by > the builder. Thus I suggested that the builder first checks if it already has > the resource loaded before it adds the resource obtained from the loader. Hope > that was clearer :-) This sounds like a serious problem to me but I'm probably wrong. I didn't study the cross linking parallelization in detail but I assume that you do process a bunch of resources with the following logic (in the second build stage): 1) load a bunch of resource in parallel until the clustering policy kicks in (without proxy resolution) 2) put all of them into the same resource set. 3) resolve the links of these resources (probably in parallel) 4) if an object has to be resolved, the usualy #getResource of the synchronized resource set is used. If two proxies trigger a resource load at the same time, the second one has to wait. This should ensure that you don't end up with different resources with the same URI. Does that make sense or am I on the wrong track? > > +1. But the optimization which I think should be removed is the method > AbstractResourceLoader#loadResource() which also tries to load resources from > the builder's resource set. > > There are probably too many patches in circulation now, but I will add one last > patch which covers my point of view. Please flag potentially obsolete patches as such.
(In reply to comment #26) > Created attachment 204171 [details] > the right patch > > I uploaded the wrong file, this one has the optimization removed. OK. I was a bit confused there :-) This patch looks exactly right to me. The only two details I would change: 1. In MANIFEST.MF add the x-friends directive to the exported resourceloader package. We can always remove it later again. 2. In SharedModule set parallel to "false" for now. Then I think we should be ready to commit the patch, as it should all still work the same way as it currently does. With that as a basis we can still discuss and work out all the details (e.g. flag to enable parallel loading for a language). Any other thoughts on this?
(In reply to comment #28) > OK. I was a bit confused there :-) This patch looks exactly right to me. The > only two details I would change: > > 1. In MANIFEST.MF add the x-friends directive to the exported resourceloader > package. We can always remove it later again. > 2. In SharedModule set parallel to "false" for now. > > Then I think we should be ready to commit the patch, as it should all still > work the same way as it currently does. With that as a basis we can still > discuss and work out all the details (e.g. flag to enable parallel loading for > a language). Any other thoughts on this? +1 Lieven could you please add the necessary comments to this bugzilla ticket according to http://www.eclipse.org/tm/development/committer_howto.php#external_contrib Knut, please mark the final patch with the iplog flag and push it if everything looks ok from your side, too.
(In reply to comment #29) > Knut, please mark the final patch with the iplog flag and push it if everything > looks ok from your side, too. Apparently that's not sufficient since the current patch exceeds the 250 lines limit. Before we can commit this, we'll have to file a CQ. If the contribution is more than 250 lines of code and the contributor is not from your own company, or there is any uncertainty about licensing and purity of IP and copyrights: fill out a Contribution Questionnaire (available from the Portal) see here, too: http://www.eclipse.org/tm/development/committer_howto.php#external_contrib
Another quote from http://www.eclipse.org/tm/development/committer_howto.php#external_contrib: Once IP due diligence is completed: Apply the patch and commit it. Do not make local modifications between applying and committing, in order to keep the process transparent. So I will make the two modifications I proposed (adding x-friends and disabling parallel loading for now) after committing the patch. Alternatively you could update the patch again :-)
attachment 204171 [details] I, Lieven Lemiengre, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.
Patch pushed to MASTER. Thank you for the patch, Lieven! I applied some post commit changes (as a separate Git commit): - added a contributors section to the new files - disabled the parallel loading by default (as agreed) - added an x-friends directive to the MANIFEST.MF for the new package (as agreed)
Closing all bugs that were set to RESOLVED before Neon.0
