You are here: Foswiki>Tasks Web>Item11929 (02 Dec 2012, GeorgeClark)Edit Attach

Item11929: Serious performance problem when deleting topics

pencil
Priority: Urgent
Current State: Closed
Released In: 1.1.6
Target Release: patch
Applies To: Engine
Component: FoswikiUIRename, Performance
Branches: Release01x01 trunk
Reported By: MichaelDaum
Waiting For:
Last Change By: GeorgeClark
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 frown, sad smile

-- 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
 

ItemTemplate edit

Summary Serious performance problem when deleting topics
ReportedBy MichaelDaum
Codebase 1.1.5, 1.1.5 RC2, 1.1.5 RC1, 1.1.4, 1.1.4 RC2, 1.1.4 RC1, 1.1.4 beta2, 1.1.4 beta1, 1.1.3, 1.1.3 RC1, 1.1.3 beta1, 1.1.2, 1.1.1, trunk
SVN Range
AppliesTo Engine
Component FoswikiUIRename, Performance
Priority Urgent
CurrentState Closed
WaitingFor
Checkins distro:4a646d02a9ed distro:7b2e8dac7cf6 distro:c4b5aa7c3043 distro:380afb2abb47
TargetRelease patch
ReleasedIn 1.1.6
CheckinsOnBranches Release01x01 trunk
trunkCheckins distro:4a646d02a9ed distro:7b2e8dac7cf6
Release01x01Checkins distro:c4b5aa7c3043 distro:380afb2abb47
Topic revision: r21 - 02 Dec 2012, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy