Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Modeling » EMF » [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database
[CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #656810] Mon, 28 February 2011 17:15 Go to next message
Gergely Nagy is currently offline Gergely Nagy
Messages: 9
Registered: July 2009
Location: Budapest, Hungary
Junior Member
While trying to delete an item from a CDO resource, I found that the whole resource contents was loaded into memory. This is less than ideal for the 300.000> elements at hand...

Some debugging revealed the following:

1) remove(Object) on the resource contents calls indexOf(Object), which basically compares the to-be-removed element to all others in the list.

2) Once the element is removed from it's container, the reference is also unset in the other direction in InternalEOBjectImpl.eSetResource(). The EObject tries to remove itself again from it's container (however since it is no longer in that list, removal doesn't occur and we don't get into an endless loop).

3) In DelegatingEcoreEList.indexOf(Object) if the element is not found in the list, each element is resolved and compared again. If the item is really not in the list, the iteration stops only when the end of the list is reached.

4) Combine 2) and 3), and BAM, the whole resource is loaded.

I reckon if remove() is not called the second time, the whole loading will not occur for the above scenario. I haven't traced it, but I assume the same holds true for any one-to-many containment relationship that is also bidirectional, not just resource contents.

If the element never existed in the list in the first place, the proxy proxy loading can not be avoided. Or can it? I'm wondering if for resources proxy resolution is necessary at all for indexOf(Object). In order to get a reference to the object, it needs to be looked up. The only occasion I can think of when a lookup returns a proxy is if the element is not contained in any of the resources in the resource set (dangling URI somewhere), in which case indexOf() should just return -1.

Is this a bug to be filed?

Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #656834 is a reply to message #656810] Mon, 28 February 2011 18:16 Go to previous messageGo to next message
Eike Stepper is currently offline Eike Stepper
Messages: 5545
Registered: July 2009
Senior Member
Hi Greg,

I've not yet heard about this issue, so, without further investigation, I can hardly know what's going on there. I wonder if, what you describe, is the normal EMF behaviour and is "just" not optimal in a lazy loading environment. Due to my EclipseCon preparations my time is a little limited until end of March, but a bugzilla makes perfect sense.

In the meantime you could try to regenerate your model with Resolve Proxies=false. CDO does not rely on EMF proxies.

Cheers
/Eike

----
http://www.esc-net.de
http://thegordian.blogspot.com
http://twitter.com/eikestepper



Am 28.02.2011 18:16, schrieb Gergely Nagy:
> While trying to delete an item from a CDO resource, I found that the whole resource contents was loaded into memory. This is less than ideal for the 300.000> elements at hand...
>
> Some debugging revealed the following:
>
> 1) remove(Object) on the resource contents calls indexOf(Object), which basically compares the to-be-removed element to all others in the list.
>
> 2) Once the element is removed from it's container, the reference is also unset in the other direction in InternalEOBjectImpl.eSetResource(). The EObject tries to remove itself again from it's container (however since it is no longer in that list, removal doesn't occur and we don't get into an endless loop).
>
> 3) In DelegatingEcoreEList.indexOf(Object) if the element is not found in the list, each element is resolved and compared again. If the item is really not in the list, the iteration stops only when the end of the list is reached.
>
> 4) Combine 2) and 3), and BAM, the whole resource is loaded.
>
> I reckon if remove() is not called the second time, the whole loading will not occur for the above scenario. I haven't traced it, but I assume the same holds true for any one-to-many containment relationship that is also bidirectional, not just resource contents.
>
> If the element never existed in the list in the first place, the proxy proxy loading can not be avoided. Or can it? I'm wondering if for resources proxy resolution is necessary at all for indexOf(Object). In order to get a reference to the object, it needs to be looked up. The only occasion I can think of when a lookup returns a proxy is if the element is not contained in any of the resources in the resource set (dangling URI somewhere), in which case indexOf() should just return -1.
>
> Is this a bug to be filed?
>
> Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #656851 is a reply to message #656810] Mon, 28 February 2011 18:58 Go to previous messageGo to next message
Ed Merks is currently offline Ed Merks
Messages: 26152
Registered: July 2009
Senior Member
Greg,

I'm trying to figure out if there are CDO-specific issues here or
something more general.


Gergely Nagy wrote:
> While trying to delete an item from a CDO resource, I found that the
> whole resource contents was loaded into memory. This is less than
> ideal for the 300.000> elements at hand...
>
> Some debugging revealed the following:
>
> 1) remove(Object) on the resource contents calls indexOf(Object),
> which basically compares the to-be-removed element to all others in
> the list.
Yes, though it uses == comparison, which is very fast, and of course you
could use remove(int) instead.
>
> 2) Once the element is removed from it's container,
You mean the container or the resource? Is cross resource containment
involved?
> the reference is also unset in the other direction in
> InternalEOBjectImpl.eSetResource().
You mean BasicEObjectImpl... Yes, removing from a resource requires the
object's pointer at the resource to be updated.
> The EObject tries to remove itself again from it's container
You mean from the resource? I.e., this **line.

Resource.Internal oldResource = eDirectResource();
if (oldResource != null)
{
** notifications =
((InternalEList<?>)oldResource.getContents()).basicRemove(this,
notifications);
> (however since it is no longer in that list, removal doesn't occur and
> we don't get into an endless loop).
Hmmm. I wonder if we need to check in the case that the new resource is
null...
>
> 3) In DelegatingEcoreEList.indexOf(Object) if the element is not found
> in the list, each element is resolved and compared again. If the item
> is really not in the list, the iteration stops only when the end of
> the list is reached.
Is that used by CDO for the resource's contents? In the standard
runtime's implementation, a resource's contents can't be proxies...
>
> 4) Combine 2) and 3), and BAM, the whole resource is loaded.
>
> I reckon if remove() is not called the second time, the whole loading
> will not occur for the above scenario. I haven't traced it, but I
> assume the same holds true for any one-to-many containment
> relationship that is also bidirectional, not just resource contents.
No.
>
> If the element never existed in the list in the first place, the proxy
> proxy loading can not be avoided. Or can it? I'm wondering if for
> resources proxy resolution is necessary at all for indexOf(Object).
I don't expect resources to ever contain proxies directly; the standard
runtime doesn't support.
> In order to get a reference to the object, it needs to be looked up.
> The only occasion I can think of when a lookup returns a proxy is if
> the element is not contained in any of the resources in the resource
> set (dangling URI somewhere), in which case indexOf() should just
> return -1.
>
> Is this a bug to be filed?
>
> Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #656962 is a reply to message #656810] Tue, 01 March 2011 08:03 Go to previous messageGo to next message
Eike Stepper is currently offline Eike Stepper
Messages: 5545
Registered: July 2009
Senior Member
I've filed bug 338508: CDOResource.contents should not resolve proxies
https://bugs.eclipse.org/bugs/show_bug.cgi?id=338508

Cheers
/Eike

----
http://www.esc-net.de
http://thegordian.blogspot.com
http://twitter.com/eikestepper



Am 28.02.2011 18:16, schrieb Gergely Nagy:
> While trying to delete an item from a CDO resource, I found that the whole resource contents was loaded into memory. This is less than ideal for the 300.000> elements at hand...
>
> Some debugging revealed the following:
>
> 1) remove(Object) on the resource contents calls indexOf(Object), which basically compares the to-be-removed element to all others in the list.
>
> 2) Once the element is removed from it's container, the reference is also unset in the other direction in InternalEOBjectImpl.eSetResource(). The EObject tries to remove itself again from it's container (however since it is no longer in that list, removal doesn't occur and we don't get into an endless loop).
>
> 3) In DelegatingEcoreEList.indexOf(Object) if the element is not found in the list, each element is resolved and compared again. If the item is really not in the list, the iteration stops only when the end of the list is reached.
>
> 4) Combine 2) and 3), and BAM, the whole resource is loaded.
>
> I reckon if remove() is not called the second time, the whole loading will not occur for the above scenario. I haven't traced it, but I assume the same holds true for any one-to-many containment relationship that is also bidirectional, not just resource contents.
>
> If the element never existed in the list in the first place, the proxy proxy loading can not be avoided. Or can it? I'm wondering if for resources proxy resolution is necessary at all for indexOf(Object). In order to get a reference to the object, it needs to be looked up. The only occasion I can think of when a lookup returns a proxy is if the element is not contained in any of the resources in the resource set (dangling URI somewhere), in which case indexOf() should just return -1.
>
> Is this a bug to be filed?
>
> Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #656997 is a reply to message #656851] Tue, 01 March 2011 10:14 Go to previous messageGo to next message
Gergely Nagy is currently offline Gergely Nagy
Messages: 9
Registered: July 2009
Location: Budapest, Hungary
Junior Member
Ed,

Thanks for your reply.

Ed Merks wrote on Mon, 28 February 2011 13:58

Gergely Nagy wrote:
>> While trying to delete an item from a CDO resource, I found that the
>> whole resource contents was loaded into memory. This is less than
>> ideal for the 300.000> elements at hand...
>>
>> Some debugging revealed the following:
>>
>> 1) remove(Object) on the resource contents calls indexOf(Object),
>> which basically compares the to-be-removed element to all others in
>> the list.
>Yes, though it uses == comparison, which is very fast, and of course you
>could use remove(int) instead.

1) Is not really the issue, that comparison is in fact sufficiently fast if no proxies need to be resovled. It was just the first observation I made while debugging, that I wanted to mention separately from 2) and 3), since both are affected by it.

I tried to use remove(int) directly, but I couldn't find a good way to get that index for an object at hand. I considered a DB query to a CDO lists table, but that won't work once the DB and the transaction get out of sync as the user changes things, e.g. for the second remove before a commit...

>> 2) Once the element is removed from it's container,
> You mean the container or the resource? Is cross resource containment
> involved?

In my case container == resource. I incorrectly presumed this may also be the case for 1->* containment relationships, thus the generic phrasing...


>> the reference is also unset in the other direction in
>> InternalEOBjectImpl.eSetResource().
>You mean BasicEObjectImpl... Yes, removing from a resource requires the
>object's pointer at the resource to be updated.

>> The EObject tries to remove itself again from it's container
>You mean from the resource? I.e., this **line.
>
> Resource.Internal oldResource = eDirectResource();
> if (oldResource != null)
> {
>** notifications =
>((InternalEList<?>)oldResource.getContents()).basicRemove(this,
>notifications);

Yes, that, and also in CDOOBjectImpl.eSetResource().

>> (however since it is no longer in that list, removal doesn't occur and
>> we don't get into an endless loop).
>Hmmm. I wonder if we need to check in the case that the new resource is
>null...

If the new resource is null we still have to remove it from the old resource, and that check seems to be in order.


>> 3) In DelegatingEcoreEList.indexOf(Object) if the element is not found
>> in the list, each element is resolved and compared again. If the item
>> is really not in the list, the iteration stops only when the end of
>> the list is reached.
>Is that used by CDO for the resource's contents? In the standard
>runtime's implementation, a resource's contents can't be proxies...

Yes. I see Eike is already addressing this.


>> 4) Combine 2) and 3), and BAM, the whole resource is loaded.
>>
>> I reckon if remove() is not called the second time, the whole loading
>> will not occur for the above scenario. I haven't traced it, but I
>> assume the same holds true for any one-to-many containment
>> relationship that is also bidirectional, not just resource contents.
>No.

I have debugged it now, and indeed, for non-resource containment there is no second remove, emf inverseRemove never turns into a full remove.

However, ContentCDOList.inverseRemove() calls eObject.eSetResource, which should be eBasicSetContainer() according to the above. So then in 2) eSetResource is working correctly, but it should not be called.

>> If the element never existed in the list in the first place, the proxy
>> proxy loading can not be avoided. Or can it? I'm wondering if for
>> resources proxy resolution is necessary at all for indexOf(Object).
>I don't expect resources to ever contain proxies directly; the standard
>runtime doesn't support.
> In order to get a reference to the object, it needs to be looked up.
> The only occasion I can think of when a lookup returns a proxy is if
> the element is not contained in any of the resources in the resource
> set (dangling URI somewhere), in which case indexOf() should just
> return -1.

Ok thanks, I wasn't sure about this.


I'll try out Eike's fix for never resolving proxies in CDO resources. What about ContentCDOList.inverseRemove()? Is there a bug as well?

Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #657001 is a reply to message #656997] Tue, 01 March 2011 10:33 Go to previous messageGo to next message
Eike Stepper is currently offline Eike Stepper
Messages: 5545
Registered: July 2009
Senior Member
Am 01.03.2011 11:14, schrieb Gergely Nagy:
> Ed,
>
> Thanks for your reply.
>
> Ed Merks wrote on Mon, 28 February 2011 13:58
>
> Gergely Nagy wrote:
>>> While trying to delete an item from a CDO resource, I found that the whole resource contents was loaded into memory. This is less than ideal for the 300.000> elements at hand...
>>>
>>> Some debugging revealed the following:
>>>
>>> 1) remove(Object) on the resource contents calls indexOf(Object), which basically compares the to-be-removed element to all others in the list.
>> Yes, though it uses == comparison, which is very fast, and of course you could use remove(int) instead.
>
> 1) Is not really the issue, that comparison is in fact sufficiently fast if no proxies need to be resovled. It was just the first observation I made while debugging, that I wanted to mention separately from 2) and 3), since both are affected by it.
> I tried to use remove(int) directly, but I couldn't find a good way to get that index for an object at hand. I considered a DB query to a CDO lists table, but that won't work once the DB and the transaction get out of sync as the user changes things, e.g. for the second remove before a commit...
I don't suggest that a server round trip is a good solution here but please note that CDO's IQueryHandlers can now consider the client's dirty state on the server side. The OCLQueryHandler does that for example if the query has been created on a CDOTransaction.

>
>>> 2) Once the element is removed from it's container,
>> You mean the container or the resource? Is cross resource containment involved?
>
> In my case container == resource. I incorrectly presumed this may also be the case for 1->* containment relationships, thus the generic phrasing...
>
>
>>> the reference is also unset in the other direction in InternalEOBjectImpl.eSetResource().
>> You mean BasicEObjectImpl... Yes, removing from a resource requires the object's pointer at the resource to be updated.
>
>>> The EObject tries to remove itself again from it's container
>> You mean from the resource? I.e., this **line.
>>
>> Resource.Internal oldResource = eDirectResource();
>> if (oldResource != null)
>> {
>> ** notifications = ((InternalEList<?>)oldResource.getContents()).basicRemove(this, notifications);
>
> Yes, that, and also in CDOOBjectImpl.eSetResource().
>
>>> (however since it is no longer in that list, removal doesn't occur and we don't get into an endless loop).
>> Hmmm. I wonder if we need to check in the case that the new resource is null...
>
> If the new resource is null we still have to remove it from the old resource, and that check seems to be in order.
>
>
>>> 3) In DelegatingEcoreEList.indexOf(Object) if the element is not found in the list, each element is resolved and compared again. If the item is really not in the list, the iteration stops only when the end of the list is reached.
>> Is that used by CDO for the resource's contents? In the standard runtime's implementation, a resource's contents can't be proxies...
>
> Yes. I see Eike is already addressing this.
Yes, i've committed that fix for both CDOResource.contents and CDOResourceFolder.nodes.

>
>
>>> 4) Combine 2) and 3), and BAM, the whole resource is loaded.
>>>
>>> I reckon if remove() is not called the second time, the whole loading will not occur for the above scenario. I haven't traced it, but I assume the same holds true for any one-to-many containment relationship that is also bidirectional, not just resource contents.
>> No.
>
> I have debugged it now, and indeed, for non-resource containment there is no second remove, emf inverseRemove never turns into a full remove.
>
> However, ContentCDOList.inverseRemove() calls eObject.eSetResource, which should be eBasicSetContainer() according to the above. So then in 2) eSetResource is working correctly, but it should not be called.
>
>>> If the element never existed in the list in the first place, the proxy proxy loading can not be avoided. Or can it? I'm wondering if for resources proxy resolution is necessary at all for indexOf(Object).
>> I don't expect resources to ever contain proxies directly; the standard runtime doesn't support.
>> In order to get a reference to the object, it needs to be looked up. The only occasion I can think of when a lookup returns a proxy is if the element is not contained in any of the resources in the resource set (dangling URI somewhere), in which case indexOf() should just return -1.
>
> Ok thanks, I wasn't sure about this.
>
>
> I'll try out Eike's fix for never resolving proxies in CDO resources. What about ContentCDOList.inverseRemove()? Is there a bug as well?
>
> Greg
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #906782 is a reply to message #657001] Sun, 02 September 2012 14:39 Go to previous messageGo to next message
Andras Peteri is currently offline Andras Peteri
Messages: 6
Registered: January 2010
Junior Member
Hi,

We've changed our code to use remove(int) in the meantime, but still seeing the same problem because of the previously discussed code segment in CDOObjectImpl.eSetResource, line 742:

if (oldResource != null)
{
  notifications = ((InternalEList<?>)oldResource.getContents()).basicRemove(this, notifications);

  // When setting the resource to null we assume that detach has already been called in the resource implementation
  if (!isSameView && resource != null)
  {
    oldResource.detached(this);
  }
}


The parallel class of EMF, BasicEObjectImpl has been changed in the version shipping with Juno to the following:

// When setting the resource to null we assume that detach has already been called in the resource implementation
//
if (oldResource != null && resource != null)
{
  notifications = ((InternalEList<?>)oldResource.getContents()).basicRemove(this, notifications);
  oldResource.detached(this);
}


...which seems to be the realisation of the idea Ed was contemplating in his reply above ("I wonder if we need to check in the case that the new resource is null..."). eSetResource should only be called from inverseAdd/inverseRemove in the appropriate ContentsEList implementation (current javadoc only mentions inverseAdd), and in the latter case, there's no need to attempt removing the object in question from its old resource twice - the second remove operation is effectively a no-op, as the object is no longer a member of the list, but it results in mass loading of revisions, as all of the remaining members (which are kept as a partially populated list of CDOIDs in the EStore implementation) have to be loaded, converted into EObjects, and only then checked for reference equality.

My suggestion would be to update CDOObjectImpl in a similar way as it was done in BasicEObjectImpl; I'd love to post a patch for this, but am not sure where the "isSameView" checks would have to be placed, which complicates matters a bit in CDO's implementation.
Re: [CDO] Resource.getContents().remove(Object) unnecessarily loads all elements from database [message #911277 is a reply to message #906782] Tue, 11 September 2012 09:27 Go to previous message
Eike Stepper is currently offline Eike Stepper
Messages: 5545
Registered: July 2009
Senior Member
Am 02.09.2012 16:39, schrieb Andras Peteri:
> Hi,
>
> We've changed our code to use remove(int) in the meantime, but still seeing the same problem because of the previously
> discussed code segment in CDOObjectImpl.eSetResource, line 742:
>
>
> if (oldResource != null)
> {
> notifications = ((InternalEList<?>)oldResource.getContents()).basicRemove(this, notifications);
>
> // When setting the resource to null we assume that detach has already been called in the resource implementation
> if (!isSameView && resource != null)
> {
> oldResource.detached(this);
> }
> }
>
>
> The parallel class of EMF, BasicEObjectImpl has been changed in the version shipping with Juno to the following:
>
>
> // When setting the resource to null we assume that detach has already been called in the resource implementation
> //
> if (oldResource != null && resource != null)
> {
> notifications = ((InternalEList<?>)oldResource.getContents()).basicRemove(this, notifications);
> oldResource.detached(this);
> }
>
>
> ..which seems to be the realisation of the idea Ed was contemplating in his reply above ("I wonder if we need to check
> in the case that the new resource is null..."). eSetResource should only be called from inverseAdd/inverseRemove in
> the appropriate ContentsEList implementation (current javadoc only mentions inverseAdd), and in the latter case,
> there's no need to attempt removing the object in question from its old resource twice - the second remove operation
> is effectively a no-op, as the object is no longer a member of the list, but it results in mass loading of revisions,
> as all of the remaining members (which are kept as a partially populated list of CDOIDs in the EStore implementation)
> have to be loaded, converted into EObjects, and only then checked for reference equality.
>
> My suggestion would be to update CDOObjectImpl in a similar way as it was done in BasicEObjectImpl; I'd love to post a
> patch for this, but am not sure where the "isSameView" checks would have to be placed, which complicates matters a bit
> in CDO's implementation.
Thanks for the offer. Looking at the code I thought the fix could be easy. I've filed this bug:

389231: Don't load all resource contents for remove(int)
https://bugs.eclipse.org/bugs/show_bug.cgi?id=389231

And fixed it. Please cc yourself and tell whether it helps you.

Cheers
/Eike

----
http://www.esc-net.de
http://thegordian.blogspot.com
http://twitter.com/eikestepper
Previous Topic:DoubleClick with key pressing
Next Topic:[CDO] Deadlock/Memory leak on multiple concurrent transactions?
Goto Forum:
  


Current Time: Fri Oct 31 22:23:10 GMT 2014

Powered by FUDForum. Page generated in 0.01982 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software