Priority: Urgent
Current State: Closed
Released In: 1.1.6
Target Release: patch
Applies To: Engine
Component: FoswikiUIRename, Performance
Branches: Release01x01 trunk
Observation: deleting a topic (or moving a topic accross webs) takes forever
Normally
Foswiki::UI::Rename
gets the list of all referring topics as an url parameter.
This is the list of topics that the core must process while rewriting references ... in theory.
Alas, it actually tries to inspect
all topics of the source web and tries to rewrite internal references.
Worse, for each topic of the source web there's a nested loop over each topics of the target web trying to
rewrite the content....
this even though the rename cgi received an empty list of referring topics.
This is a serious performance issue when both webs - target and source web - have a large number of topics in it.
The error seems to be in
_moveTopicOrAttachment
calling
_replaceWebInternalReferences
without using the
$refs
array
to narrow down the scope of processing...
Out of pure desperation I disabled the call to
_replaceWebInternalReferenecs
being called when moving a topic across webs.
Hot fix for lib/Foswiki/UI/Rename.pm:
@@ -785,7 +783,8 @@
# If the web changed, replace local refs to the topics
# in $from->web with full $from->web.topic references so that
# they still work.
- _replaceWebInternalReferences( $session, $from, $to );
+# _replaceWebInternalReferences( $session, $from, $to );
}
First test show that references are still fixed...
So WTF is this code doing?
Help.
--
MichaelDaum - 06 Jun 2012
I think that this might be a duplicate of
Item10222, and you seem to have narrowed it down to the problem code.
Note with the above line of code commented out, one of the
RenameTests fails:
RenameTests::test_renameTopic_new_web_same_topic_name
--
GeorgeClark - 07 Jun 2012
Such an old bug so urgent to fix ... All 1.1.x engines are affected. This error is known since Jan 2011
--
MichaelDaum - 07 Jun 2012
And as far as I see the unit test is actually wrong. When moving OldWeb.OldTopic to NewWeb.OldTopic it says:
OldTopic -> OldWeb.OldTopic
I can't see the sense of this tranformation...
The test should be:
--- test/unit/RenameTests.pm (revision 14949)
+++ test/unit/RenameTests.pm (working copy)
@@ -1134,7 +1134,7 @@
11 [[OldTopic][the text]]
12 $this->{test_web}.NewTopic
13 $this->{new_web}.OldTopic
-14 $this->{test_web}.OtherTopic
+14 OtherTopic
15 $this->{test_web}.OtherTopic
16 $this->{new_web}.OtherTopic
Problem is, I can't reproduce the failure using rename in real life. So it may be an artifact of the unit test all together.
--
MichaelDaum - 07 Jun 2012
Possibly. I agree with your analysis about the unit test; chances are someone avoided having to fix the problem exposed by simply changing the unit test (now, who would do something like that?)
--
CrawfordCurrie - 07 Jun 2012
Basically the analysis shows that large parts of
Foswiki::UI::Rename
is obsolete code. Disabling the line above in this class and in the unit test will dry out most of these nested calls to iterator classes causing this performance problem. This is lots of code. Therefore I'd really like to hear why this code was written.
Otherwise I'll remove this code next week.
--
MichaelDaum - 08 Jun 2012
It appears that this code was refactored out of Manage.pm and Render.pm in the big API refactor in
distro:142fd927aa4a, from March 2009. I think the unit tests are pretty thorough, and that one particular failing test appears to be bogus. I'd recommend trunk only though. Unless someone actually understands what the code is doing and why it's there, it seems a bit risky for 1.1.6.
--
GeorgeClark - 08 Jun 2012
This error is pretty urgent. Rename basically is out of order on bigger foswikis. So a fix should be out part of 1.1.6. Another release with this bug unfixed would be a shame.
--
MichaelDaum - 11 Jun 2012
There's more wrong in the unit tests. There's a test to check renaming of
Web/OldTopic ->
Web/NewTopic. I am not sure whether this is supposed to work at all. For now rename is only able to fix slash delimiters when the whole thing is an url, but not a bracket link.
This test fails before and after my changes.
I leave this test failing for now.
--
MichaelDaum - 11 Jun 2012
I have experienced timeouts when renaming so I agree with Michael that this is 1.1.6 must have.
--
KennethLavrsen - 11 Jun 2012
The unit test "failure" is actually a pass. It's an "expected failure" that is not counted as a fail. There was already a smell in that test just a few lines further down:
# SMELL: If this gets fixed, then the expected line 37 needs to be changed
# in test_renameTopic_same_web_new_topic_name.
$this->expect_failure();
$this->annotate("[[Web/Topic]] fails");
The unit test was written for
Item11555 which is not yet fixed. The task probably should be listed in the SMELL
As far as getting this fix into 1.1.6, I agree. I was only expressing concern that the code was being deleted without anyone reporting an understanding of why it was there. Once it cooks in trunk for a while without introducing new issues, it should go into 1.1.6.
--
GeorgeClark - 11 Jun 2012
Understood. So last pending action for this bug item is merging it to the 1.1.x branch.
--
MichaelDaum - 13 Jun 2012
The checkins have been cherry-picked to Release 1.1. Note, to make it clear that merging is required,
CurrentState should be set to
Needs Merge
.
--
GeorgeClark - 28 Jul 2012
See
ChangeRenameBehaviourOnRenamingLinks
--
MichaelDaum - 30 Jul 2012
I've added this to the
KnownIssuesOfFoswiki01x01, which includes a hotfix suggestion.
--
PaulHarvey - 28 Sep 2012