[ROOT CAUSE]
When there are too many Name Ranges in the ODF file, it takes a very long time to import the document.

By using Purify, we found most of the time is used by function ScRangeData::Clone(), which is called by function ScCollection::= while constructing a new ScRangeName object.

Function:	ScRangeData::Clone
Calls:	1,511,522
Function time:	140,541.25 (3.08% of Focus)
F+D time:	845,851.27 (18.51% of Focus)

In my test, there is 1004 name ranges. In funtion ScXMLImport::SetNamedRanges(), it will first add 1004 new Name Ranges, then it will loop the 1004 entries to update the content and grammer. You can see that 1004 Name Ranges will lead to 1.5 Million (roughly 1004*1004) calls of ScRangeData::Clone(), which is quite time consuming, thus resulting a slow importing of the document.

Function:	ScXMLImport::SetNamedRanges
Calls:	1

Descendant
ScNamedRangesObj::addNewByName	                calls:1004
ScNamedRangeObj::SetContentWithGrammar	calls:1004

1) create new name ranges
Function:	ScNamedRangesObj::addNewByName
Calls:	1,004

Descendant
ScNamedRangesObj::ImplAddNewByScopeAndName	calls: 1004


2) update the name ranges
Function:	ScNamedRangeObj::SetContentWithGrammar
Calls:	1,004

Descendant
ScNamedRangeObj::Modify_Impl	calls: 1004

[RESOLUTION]
To improve the performance,  we will not create a copy of the ScNameRange object, instead we will just use the existing ScNameRange to update.

Created attachment 82330 [details]
Fix

Can you explain why creating the clones is not necessary and why it is safe to share the underlying ScRangeName?

(In reply to Andre from comment #3)
> Can you explain why creating the clones is not necessary and why it is safe
> to share the underlying ScRangeName?

Hi Andre,

1. Firstly, the clone is quite time consuming, so we should try to avoid it as possible as we can.

Takes 1000 name ranges for example, there will be 1000*1000 steps to process.

outer loop /* 1000 entries */
  clone (inner loop /* 1000 entries */)


virtual	ScDataObject*     Clone(ScDocument* pDocP) const
{ return new ScRangeName(*this, pDocP); }


ScRangeName::ScRangeName(const ScRangeName& rScRangeName, ScDocument* pDocument) :
				ScSortedCollection ( rScRangeName ),
				pDoc ( pDocument ),
				nSharedMaxIndex (rScRangeName.nSharedMaxIndex)
{
	for (USHORT i = 0; i < nCount; i++)
	{
		((ScRangeData*)At(i))->SetDocument(pDocument);
		((ScRangeData*)At(i))->SetIndex(((ScRangeData*)rScRangeName.At(i))->GetIndex());
	}
}


2. For this case, we have got all the name ranges, and it is one of the document's member variables. We can just manipulate it directly, and there is no need to clone a new ScRangeName object. All the operations done on the name ranges object is within the document object, and there is no other side effect. So I think it is safe to do that.

 ScRangeName* pNames = pDoc->GetRangeName();

I totally agree that avoiding the cloning would be a good idea.
I am just trying to make sure that there are no side effects when the cloning is removed.

The method GetRangeName is defined in sc/inc/document.hxx as
    public:
    ...
    SC_DLLPUBLIC ScRangeName*	GetRangeName();
so it is not only public but also accessible from outside of the sc module.  That means that when the returned ScRangeName object is modified because it is not cloned anymore and local copies are modified, then that can cause subtle side effects in and outside of sc.

If that is just a hypothetical problem (because nobody calls GetRangeName() at the moment, or does not store a pointer to the returned ScRangenName object or does not expect it not to change) then it might be best to avoid future problems in this area by
- Documenting clearly in sc/in/document.hxx that the returned ScRangeName is volatile, ie can change at any time.
- Removing the SC_DLLPUBLIC specifier (if possible)
- Making it private (if possible)

Hi Andre,

- Documenting clearly in sc/in/document.hxx that the returned ScRangeName is volatile, ie can change at any time.

I've added comments in document.hxx for this. And the patch is updated.

- Removing the SC_DLLPUBLIC specifier (if possible)
This might be possible. My concerns are there may be some existing extensions of AOO that possibly called GetRangeName(). If we disable the DLL export, they might be broken. So I vote to preserve this in the code.

- Making it private (if possible)
This seems not possible, because there are lots calls of GetRangeName() in other modules already.

Created attachment 82381 [details]
Fix2

This is an update of previous patch, adding comments in document.hxx to note that the Name Range get from document is volatile.

(In reply to wujinlong from comment #6)
> Hi Andre,
> 
> - Documenting clearly in sc/in/document.hxx that the returned ScRangeName is
> volatile, ie can change at any time.

Checking that the calling code can handle this volatility is part of the change, right?

> 
> I've added comments in document.hxx for this. And the patch is updated.
> 
> - Removing the SC_DLLPUBLIC specifier (if possible)
> This might be possible. My concerns are there may be some existing
> extensions of AOO that possibly called GetRangeName(). If we disable the DLL
> export, they might be broken. So I vote to preserve this in the code.
> 
> - Making it private (if possible)
> This seems not possible, because there are lots calls of GetRangeName() in
> other modules already.

(In reply to Andre from comment #8)
> (In reply to wujinlong from comment #6)
> > Hi Andre,
> > 
> > - Documenting clearly in sc/in/document.hxx that the returned ScRangeName is
> > volatile, ie can change at any time.
> 
> Checking that the calling code can handle this volatility is part of the
> change, right?
> 

I don't see this should be part of this fix. Two reasons,

1. There is such volatility issue existing in the original code for a long time. The original code uses Clone() to make a copy of the ScRangeName object, and at last the cloned ScRangeName object is set back to the Document object.

The key point is there is performance issue in the code, and we are trying to fix that by not using Clone().

2. If we use something like lock to resolve the volatility issue, it might introduce new performance issues. And we need to do more testing to make sure of that.

So in summary, if you think we need to address this volatility issue more decently, I propose to do that in a separate task.

(In reply to wujinlong from comment #9)
> (In reply to Andre from comment #8)
> > (In reply to wujinlong from comment #6)
> > > Hi Andre,
> > > 
> > > - Documenting clearly in sc/in/document.hxx that the returned ScRangeName is
> > > volatile, ie can change at any time.
> > 
> > Checking that the calling code can handle this volatility is part of the
> > change, right?
> > 
> 
> I don't see this should be part of this fix. Two reasons,

It should be if the fix changes existing behavior, and I am thinking that it does.

> 
> 1. There is such volatility issue existing in the original code for a long
> time. The original code uses Clone() to make a copy of the ScRangeName
> object, and at last the cloned ScRangeName object is set back to the
> Document object.

OK, I agree that that takes care of the volatility issue.  But if I am reading the code correctly, the cloned or modified ScRangeName is set via ScDocFunc::SetNewRangeNames().  There an undo object is created.  Without cloning the ScRangeName object, it appears that the undo object references the same ScRangeName twice.  This would make the undo operation useless.
Any idea how to deal with that?

> 
> The key point is there is performance issue in the code, and we are trying
> to fix that by not using Clone().

I want to prevent that we make trade a speed gain for a loss of functionality.

> 
> 2. If we use something like lock to resolve the volatility issue, it might
> introduce new performance issues. And we need to do more testing to make
> sure of that.
> 
> So in summary, if you think we need to address this volatility issue more
> decently, I propose to do that in a separate task.

Hi Andre,

Your finding is right. My fix will cause problem with the undo operation.

I thought a new way to handle that. Since this problem only happens when opens an ODS file, I will not clone the ScRangeName object for importing the ODS file only. This is determined by ScDocument::IsImportingXML().

I've updated the patch. Please take a look at it. 

Thanks!

Created attachment 82409 [details]
New Fix

Don't do clone for importing of ODS file.

I don't like the introduction of another special case but I can't think of a better solution.  I would just ask you to turn the lines

    if ( !pDoc->IsImportingXML() )

into something like 

    const bool bSupportUndo ( ! pDoc->IsImportingXML());
    if (bSupportUndo)

just to make the reason for the special case explicit.

Comment on attachment 82409 [details]
New Fix

Review granted.

Created attachment 82555 [details]
Final Fix

Andre,

Thanks for your kind review and comments. I agree this solution is not a decent one, but just have no other better solution.

I have modified the code according to your last comment, and updated the patch.

Thank you!

"steve_y" committed SVN revision 1568219 into trunk:
Bug 124065 - [Performance] Low performance opening attached .ods

About root cause and solution please see comment 1 by Jinlong Wu

Verified on Windows 7 on AOO410ml4(Build:9750)  -  Rev. 1583418