Tags:
create new tag
, view all tags

Refactoring Proposal: Location Location Location

The changes described in this patch have been checked into the DEVELOP branch as version 1771, so I have refactored this topic accordingly.

As far as the end user is concerned there should be no functionality changes from this code change. There is one known rendering change; when converting to entities for rendering, the code will no longer generate symbolic entities (such as &) but will always use decimal entities. Leading zeroes on decimal entity numbers have also been supressed i.e. FF will be rendered as  instead of  Other than that, HTML output should be identical to the output from 1767 (the last MAIN version merged to the branch), except where the changes have fixed bugs in that version (rendering of tags, syntax of tag parameters).

Summary of change:

  1. Moves functionality that knows how %META is interleaved with text into Store, leaving MetaDotPm "pure".
  2. Moves meta rendering functionality out of TWikiDotPm into RenderDotPm
  3. Reverse-engineered documentation for MetaDotPm
  4. Converted several StoreDotPm functions to PRIVATE (by adding preceding underscore).
  5. Tidies up documentation so it better reflects external interfaces to StoreDotPm and MetaDotPm
  6. Tidies up ViewDotPm, which I discovered was reading the topic twice for old revs for no good reason. Though I might have missed something.
    • There is a reason: Security and performance. The topic text of the top revision is known at the time of the access check, thus fast for the most common topic view (top revision). This also makes sure that an older topic rev cannot be seen if access restriction has been added later on. The topic is read again for a revision older then the top, which is neglectable since it is the exception. -- PTh
  7. All log files can have %DATE% in name (doesn't affect current function)
    • Can you be more specific? -- PTh
  8. Moved TOC to DefaultPlugin (temporary, I guess)
    • This if not acceptable for MAIN, it slows down the rendering. As stated in MoveTOCToAPlugin, TOC is a core functionality; it can be overloaded by a Plugin if needed -- PTh
  9. Moved email list discovery into mailnotify.txt. This functionality should move to the User.pm module when it is refactored.
  10. Converted Attach.pm to use hashes instead of arrays (faster)
    • Does the sequence of files in the attachment table change? Currently it is by upload sequence. OK by me if spec change, e.g. sorted, but should be stated. -- PTh
  11. %ATTACHURLPATH%/filename legacy handling moved to DefaultPlugin
    • What legacy handling? Why in DefaultPlugin? -- PTh
  12. Removed several pointless functions from TWiki.pm, to eliminate compile and call overheads.
    • Can you be more specific? Which ones? Removed or moved elsehere? -- PTh
  13. Recoded TWiki.pm to parse %TOKEN syntax variables and dispatch them to handlers. This allows a single-pass algorithm for processing the text. This has the impact that the expansion order has changed; tags are parsed, and tags in parameters to other tags are fully expanded before the values are used. This may break some TWiki applications, but it is improbable as the fix mechanises what was attempted through the original ordering of tag expansions.
    • This will break some TWikiApplications. The rendering sequence has been crafted carefully. Just one example, static variables such as WEB and TOPIC are done before URLPARAM so that URL parameters with variables resolve correctly, and SEARCH is done after URLPARAM so that search can react to URL parameters. Any way to keep the rendering sequence? I disagree that it is OK to change the semantics to gain a few milliseconds. (in FormattedSearchWithSummary I was guilty of changing the semantics myself, but fixed it after I discovered that it broke a TWikiApplication) -- PTh

%A{"%B{"C"}%"}%

and

%B{"%A{"C"}%"}%
will both work; the inner term will always be expanded before the outer. The full rules are:
  1. Tags are expanded left to right, in the order they are encountered. Note that a parameterised tag is not ""encountered" until the matching }% has been seen, by which time all tags in parameters will have been expanded.
  2. Tags are recursively expanded as soon as they are encountered - the algorithm is inherently single-pass.
  3. Tag expansions that create new tags recursively are limited to 16 hierarchical levels of expansion.

Note in this scheme tag parameters are parsed in one place only, supporting correct handling of tag syntaxes.

Note that using this expansion scheme, it would be trivial to add the ability for plugins to register new tag handlers with the core rendering loop. This ability has been requested a number of times by various contributors.

Release Skin Plugins Time per page AthensMarks
athens   DefaultPlugin, InterwikiPlugin 0.3393 100
beijing   DefaultPlugin, InterwikiPlugin 0.3885 87
cairo   DefaultPlugin, InterwikiPlugin 0.5567 61
MAIN classic DefaultPlugin, InterwikiPlugin 0.4582 74
DEVELOP classic DefaultPlugin, InterwikiPlugin 0.4138 82
rlos classic DefaultPlugin, InterwikiPlugin 0.4054 84

Timing updated as of 30 Oct 2004 TW

-- CrawfordCurrie - 23 Oct 2004

Crawford, overall a very good refactor that makes code maintenance easier. Because of the large refactor, the changes need to be tested carefully. I am curious to learn about other code changes not mentioned in above list.

Detail when changing many extractNameValuePair calls to expandParameters: The extractNameValuePair returns an empty string if key not found, not undef. Make sure to initialize them as before, or we change the logic, introducing errors in the Apache log among other issues. E.g.

+    my $param     = $params->{_DEFAULT};
+    my $newLine   = $params->{newline};
vs. correct
-    my $param     = $params{"_DEFAULT"}  || "";
-    my $newLine   = $params{"newline"}   || "";

We can take it into MAIN once the issues are resolved.

-- PeterThoeny - 23 Oct 2004

Hmm. I checked the logic looking for cases where the difference between "" and undef would matter i.e. string values as against booleans. I thought I had caught them all. Can you point me to specific cases?

-- CrawfordCurrie - 23 Oct 2004

I have to read the changes to see where. On a large code refactor it is safer to initialize variables the same as before.

-- PeterThoeny - 23 Oct 2004

I think it is in the interest of all to have MAIN and DEVELOP not diverge too much, therefore I would like to bring them in sync sooner then later. But the mentioned issues need to be resolved first. This should have higher priority than adding new features such as GetUrlShouldSendPortNumber.

-- PeterThoeny - 27 Oct 2004

Sorry, the issue you mentioned was resolved, roughly ten minutes after I read your point. I should have updated this topic but didn't, thinking you would see the changes in the twiki-dev log. Apologies.

-- CrawfordCurrie - 27 Oct 2004

What about 6, 7, 8, 11, 12, 13?

-- PeterThoeny - 28 Oct 2004

Argh, I missed those (interleaved with existing text; we need a better "changes"....)

  • 6. I eliminated all unecessary reads in View.pm without compromising functionality.
  • 7. The pattern for specifying $debugFilename and $warningFilename in TWiki.cfg is the same as for $logFilename i.e. they can contain 2017-10-18.
  • 8. FYI moving TOC does not slow down the rendering (as it is handled in the same place as other tags in the DefaultPlugin, in exactly the same way as it would be handled if internal to core), but it does speed up the compile, fractionally. However if you insist, I will move it back.
  • 11. You had handling of %ATTACHURLPATH%/filename commented as legacy in TWiki.pm, so I moved it to where you had previously moved legacy functions i.e. to DefaultPlugin. This is one change I'm not 100% certain about, because I couldn't work out what the old code was supposed to do to my satisfaction.
  • 12. Removed. There were a number of functions that simply returned the value of a TWiki global variable, and were only called by another function that returned the value returned by the function in TWiki.pm e.g. getDataDir.
  • 13. No, there is no way to keep the current (incorrect, IMHO) rendering order. I personally doubt the change will break any TWikiApplications, because the previous careful crafting of the expansion order was an attempt to do what the code now does properly i.e. expand tags in parameter values before the enclosing tags. However it may break any TWikiApplications that have had to be crafted to overcome the existing deficiencies, but I haven't been able to find any examples yet. If you can provide one, I will happily look into it.
    If you read the code, you will understand that this refactor is not to do with gaining a few milliseconds. It restores the simple dispatcher pattern that was obvious in the architecture, but had been damaged/ignored/polluted by various hacks over time.
    BTW you can try out anything you think might be broken on the DEVELOP twiki, which was running 1788 last time I looked.
    • It's there a way to unit test this stuff (the rendering process), besides comparing the result of the base distro (Cairo) with the results of a patched distro? -- RafaelAlvarez - 28 Oct 2004

BTW, while I'm thinking about TOC, we really need a strategy that allows some plugins to register new tag types (at least simple substitutions and handlers). A simple interface of 2 functions in FuncDotPm could support this. It would speed up many plugins (they would no longer need commonTagsHandlers). These functions would work by simply adding an entry to the relevant tag hash (e.g. %dynamicInternalTags ).

# Replace occurences of %MYTAG% and %MYTAG{...}% with "MY_TEXT"
TWiki::Func::registerSimpleReplacement( "MYTAG", "MY TEXT");
# Replace occurences of %MYTAG% and %MYTAG{...}% with the string result of a call to _myFunction
TWiki::Func::registerReplacementHandler( "MYTAG", &_myFunction );

sub _myFunction {
    my ( $params, $topic, $web ) = @_; # where $params is a reference to a hash generated by extractParameters
}

-- CrawfordCurrie - 28 Oct 2004

hmm... this could really simplify the implementation of AlternatePluginTagHandling.

IIRC, some time ago Frank Carver discussed having a "pluggable" rendering pipeline in Friki, so perhaps this makes sense. In fact, it's implemented in the current version.

http://www.stringtree.org/friki/view?GenericWay

There is a lot of potential there, as people can "finetune" the rendering process removing those tags they don't need or adding the tags they feel more confortable with. Even as Twiki will be shipped with the standard set, the customer can have the power to make the choice.

-- RafaelAlvarez - 28 Oct 2004

Just thought something: If a plugin DEPENDS ON the internalTags to be already expanded (like a plugin that parses HTML tables for info), will it fail?.

-- RafaelAlvarez - 28 Oct 2004

You mean, will it fail if it registers its tags for expansion by the core loop? Possibly. If you have such a plugin, I would recommend using the commonTagsHandler/outsidePreHandler approach (though in truth I wouldn't recommend the outsidePreHandler to my worst enemy frown )

On unit testing; no. There are no tests defined for TWiki, other than the comparison tests I did. MartinCleaver and I started to capture some key test cases, and will continue to do so, but we need help, desperately.

-- CrawfordCurrie - 29 Oct 2004

I'll wait until the code is merged, take a look and then check out some "edge cases" that I can just point my finger in but are floating on my brain.

As for Test Cases for the rendering process, I have a battery of unit tests I used to test a Java-based wiki implementation. I'll translate them to perl (and in some few cases to TWikiML) and put them someplace somewhere in twiki. Expect them sometime next week.

-- RafaelAlvarez - 29 Oct 2004

13 is a sticky point. We have many TWiki apps at my workplace that do all kind of tricky things. I am almost certain that some will break if the evaluation order of variables gets changed.

I am wondering how other companies with complex TWikiApplications react to this proposed change.

-- PeterThoeny - 30 Oct 2004

Can we pull out and modularise the unlucky 13 so that people can choose which implementation they run? This will give comfort to those who think they might need the old rendering order, and if it does break, allow them to transition to the new model whilst not slowing down the rest of us.

As Crawford said, it might not break anything and we have a lot to gain.

-- MartinCleaver - 30 Oct 2004

No.

-- CrawfordCurrie - 30 Oct 2004

With regards to the "unlucky 13", I think it might affect our applications only in the following areas:

  • Using URLPARAM in search
  • Passing URLPARAM into topics to initialize forms
Knowing whether this does or does not have an impact would require studying the code, however, which I have not done.

From my point of view, I am willing to take this chance. Both situations were not really documented at the time when I "discovered" them from studying other code (I don't know whether they are documented today). Of course, I am facing all kinds of other changes due to the conversion to Cairo, so this is just another small item.

However, I would suggest that we treat these cases above as first class citizens going forward, clearly document them, and maintain them in an appropriate place. From PeterThoeny's remarks above it appears to me there might be other capabilities that might be hidden undocumented. So making this change might actually be a good thing: it will bring reliance on undocumented capabilities to the open, and will then allow us to explicitly make these capabilities available.

In summary, if we can expect a good gain in performance and in future maintainability, I am willing to take the chance on my apps. I assume I will be able to fix them.

P.S. It appears that Peter will have most of this tricky boundary cases in his applications. It would be great if we could capture those as test cases. This would also be a first start of documenting such usage for others to enjoy.

I cannot help but notice that this initiative was started by Crawford in the beginning of September. We probably should bring this to closure soon, before frustration levels grow and the branch gets too far out of synch with the trunk. Otherwise we will never be able to bring this initiative to closure.

-- ThomasWeigert - 30 Oct 2004

Neither of the areas you describe should work any differently to now. The only places where there may be some problems is where a new tag is generated as a result of concatenating two existing tags. For example, if you tried to do this, it would give %MAINWEB% and not Main:

   * Set SAUSAGE = %MAIN
   * Set MASH = WEB%
%SAUSAGE%%MASH%
Because the new tag parser is a proper parser, it is quite difficult to fox. But it is possible. If you were to do this:
%ATAG{"}%"}%
then it will fail to parse this correctly (the parser doesn't match quotes). But I think this will fail with the current REs as well.

-- CrawfordCurrie - 30 Oct 2004

I think we should go ahead with this change soon.

-- ThomasWeigert - 30 Oct 2004

Can we just rollback the render process change, put it into a separate patch, put back TOC to the core and just release this? There are 12 good enhancements that are being stopped by just 1 change for the fear of breaking something. So, my proposal to push this change forward is:

  1. Move the rendering process change into another patch.
  2. Merge the rest into Main.
  3. Release another beta.
  4. "Merge" the rendering patch in to a copy of MAIN, so Peter can test his apps and report errors .
  5. Create and/or complement the test harness to unittest the rendering process (for me, this is the most critical part to be tested).
  6. Fix anything that needs to be fixed.
  7. Merge the render patch to Main
  8. Release another beta.

What do you think?

-- RafaelAlvarez - 30 Oct 2004

The RSS feeds on DEVELOP aren't coming out right. Compare the following:

Some WikiNames are being expanded. The RSS is generated from a SEARCH with a format containing: (last changed by <nop>$wikiname) which for TWikiGuest and ArthurClemens get expanded like this:

(last changed by <span class="twikiNewLink" style='background : #FFFFCE;'><font color="#0000FF">ArthurClemens</font><sup>?</sup></span>)
but PeterThoeny is left as (last changed by PeterThoeny) which is correct.

Also, in develop rss there is an extra <P /> tag right before the last closing tag.

-- SamHasler - 30 Oct 2004

I assumed that it was a change to the way SEARCH handles newlines/separators.

-- SamHasler - 31 Oct 2004

after running some automated html comparison tests, it does look like this is coming from a change in DEVELOP (i don't know if this is a good or a bad change, but it is a difference). i haven't noticed a pattern as to which pages receive an extra <P />; more testing is required.

-- WillNorris - 30,31 Oct 2004, 02 Dec 2004

Rafael, undoing the tag handling change is undoing what I consider to be the most important refactoring. A lot of other cleanups depend on it. We might as well not bother, rather than take that back out. I'm also rather tired of this; I don't really fancy farting around any more. The sooner a beta is released with the changes in, the sooner non-developers can start to feed back problems (release early, release often).

Sam, I think your first point has been fixed by the fix to the embedded nop handling. Well spotted! I have added rss skin to the script tests.

Could you please check again, see if there's anything still obviously wrong (or if I actually fixed anything!).

-- CrawfordCurrie - 31 Oct 2004

Peter, from a user's point of view, it would be good to have more clarity on the roadmap regarding this feature. I am planning to start work on moving to Cairo. However, if I can expect the change described in this topic soon, obviously I would hold off until this change is in place. It would be very helpful to have a clear message on what will happen with respect to L^3.

As I said earlier, I am willing to take the risk on my apps in exchange for the performance and maintenance improvements. But I would prefer to make any fixes only once.

-- ThomasWeigert - 05 Nov 2004

I have corrected a typo in TWiki.pm r3181 which seems to have started in r1769. This is in the DEVELOP branch.

BTW is the summary correct/up to date? Only 5 AthensMarks? I would not want breaking any Application for 5 percent performance gain. But I think the impact is much higher!

-- FrankHartmann

Not sure what you are refering to... going from 74 AthensMarks to 82 AthensMarks is an 11% performance gain.

-- ThomasWeigert - 08 Nov 2004

Latest figures: TWiki core code benchmarks

Release Skin Plugins Time per page AthensMarks
athens   DefaultPlugin, InterwikiPlugin 0.3225 100
beijing   DefaultPlugin, InterwikiPlugin 0.3764 86
cairo   DefaultPlugin, InterwikiPlugin 0.56 56
MAIN classic DefaultPlugin, InterwikiPlugin 0.4467 72
DEVELOP classic DefaultPlugin, InterwikiPlugin 0.3962 81

i.e. a page is rendered in 90% of the time taken by MAIN, 70% of the time taken by Cairo.

-- CrawfordCurrie - 08 Nov 2004

These are very nice speed improvements.

One clarification: The Location patch is very large, it is a mix of performance improvements, code cleanup and has some spec changes. The rendering order change is a spec change that probably has very little impact on performance, that is, no big benchmark change if taken out.

The new rendering order is cleaner then the current one, the old is not very intuitive. The spec change is a good thing for people who do not need to worry about existing content.

I am holding back with merging DevelopBranch into MainBranch because at this time I do not know how many of the TWikiApplications will break at my work place. We have many apps that do all kind of obscure things, some of them are mission critical. I invited Crawford to slow down adding more features to the DevelopBranch and to install and test it at my work place. It is in everyones interest that the DevelopBranch and MainBranch do not diverge too much. The new develop model does not work as intended if it does.

Crawford: Since the rendering order change is a spec change it should be documented as a separate feature in Codev.

Thomas: Since this is a large refactor I do expect some hickups, e.g. I probably will not declare the first Beta based on this as "ready for production."

-- PeterThoeny - 08 Nov 2004

OK. See VariableHandlers. Note that the rendering order hasn't changed, only the variable expansion order. The change to the variable expansion gives about 4 AthensMarks, but it would be very difficult to restore the old expansion order now, I think.

Note also that RafaelAlvarez has spotted a major opportunity for a speedup, courtesy of ModPerlize. We are investigating.

-- CrawfordCurrie - 08 Nov 2004

-- CrawfordCurrie - 15 Nov 2004

While applying Florian Weimer's patch to DEVELOP, I came across the following confusing behaviour in TWiki::Access::checkPermissions.

Current behaviour: Iff DENYTOPIC is defined to be a non-empty value, it overrides DENYWEB Iff ALLOWTOPIC is defined to be a non-empty value, it overrides ALLOWWEB

This is confusing, as it is perfectly natural to set ALLOWTOPIC to empty in the expectation that no-one will be allowed. On the contrary, only if you set ALLOWTOPIC to "none" will this be the case.

Similarly setting DENYTOPIC to the empty value does not override DENYWEB, and doesn't remove denies as you would expect.

I have changed the behaviour on the DEVELOP codebase to:

If DENYTOPIC is defined, it overrides DENYWEB. If ALLOWTOPIC is defined, it overrides ALLOWWEB.

With the change, DEVELOP behaviour now matches the documentation. Unlike MAIN.

-- CrawfordCurrie - 29 Nov 2004

When merging a large change, tag trunk before and after the merge. That way you'll have the tags to make it easy to revert if needed. Something like:

svn copy .../trunk .../tags/LLL_Before
# do merge here
svn copy .../trunk .../tags/LLL_After

-- KennethPorter - 30 Nov 2004

Kenneth, I really can't see the point of creating a tag, when you can really easily see the revision number before and after the merge (they are after all only one number apart). This of course assuming that you are ready to do such a merge, and are unlikey to revert in most cases.

Otherwise, the Tags directory would be filled with Tags that are never used (especially as our intended process would have merges happening regularly)

so, um, please explain why, when identifying a change set is so trivial, you think Taggin is so important in a dynamic situation. (in a static one like a release, and release maintainence, I totally agree)

-- SvenDowideit - 30 Nov 2004

I guess that's true, the release number and location together are as good as a tag. It just seemed like this was effectively a "flag day", when a major change was being imposed on the code.

Note that a tag doesn't have to be in the top-level tags directory. You can reserve that for releases and put major feature tags in a subdirectory.

-- KennethPorter - 01 Dec 2004

Share your opinion of this idea by selecting one of the radio buttons below and clicking the button.
It would be nice ... 1 2 3 4 5 6 7 8 9 ... I want this so much it hurts

9 RafaelAlvarez 28 Oct 2004
9 MattWilkie 23 Oct 2004
9 AntonAylward 21 Oct 2004
9 CrawfordCurrie 15 Oct 2004
9 MartinCleaver 15 Oct 2004

Are you comfortable that, from what you have read so far, that L^3 should go into MAIN as-is, despite the tag parsing change?
Do you agree or disagree? Please select one of the radio buttons below and click the button.
I 1 - Strongly disagree   2 - Disagree   3 - am Neutral   4 - Agree   5 - Strongly Agree  


1 WillNorris 10 Nov 2004 i don't think it should go into MAIN yet, though my reasoning has nothing to do with the tag parsing change. a patch of this magnitude simply needs more testing than it has received so far. it should stay in DEVELOP until we have enough TWikiTestInfrastructure in place to better vet this patch.
5 ThomasWeigert 05 Nov 2004 Assuming that we can expect a good gain in performance and in future maintainability, I am willing to take the chance on my apps.
5 RafaelAlvarez 01 Nov 2004 We need this stuff tested out by as many people as possible (developers and end-usera alike) in the shortest time.
5 AntonAylward 01 Nov 2004 It would be logical ...
to have a test data-set thaht has every normal and syndromic (e.g. the oddball examples describned above) case, and performa full regresion test.
As it is, all we have is the actual site and a number of appliation sites.
So Matt is correct, we have to prove it by actually trying it out. The agregate benefits of the improvements are worth the effort. This is especially true of the "compile" bottle-neck.
5 MattWilkie 01 Nov 2004 put in the changes, identify exactly what TWikiApplications are broken and in what manner -- if any, so far there is only concern that it is possible to break -- and then from there it can be decided whether it's easier to fix the application or back out the change.

The changes described here are all in the DEVELOP branch. This work bridged the switchover to the "open" development model, so not everything is documented here; refer to subversion for full logs of changes.

-- CrawfordCurrie - 11 Jan 2005

ChangeProposalForm
TopicClassification CodeRefactor
TopicSummary Move methods into the right places within the core code, and adds documentation. Gives a small (5 AthensMarks) Performance boost.
CurrentState ReadyForMerge
OutstandingIssues

RelatedTopics

InterestedParties

ProposedFor DakarRelease
TWikiContributors CrawfordCurrie
Topic attachments
I Attachment History Action Size Date Who Comment
Perl source code filepm Templates.pm r1 manage 8.7 K 2004-10-06 - 09:51 CrawfordCurrie Required new file
Unknown file formatdiff reorganise.diff r14 r13 r12 r11 r10 manage 481.1 K 2004-10-22 - 16:49 CrawfordCurrie Updated for build 1767, extended test suite, fixed minor bug.
Edit | Attach | Watch | Print version | History: r91 < r90 < r89 < r88 < r87 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r91 - 2005-06-21 - SvenDowideit
 
  • Learn about TWiki  
  • Download TWiki
This site is powered by the TWiki collaboration platform Powered by Perl Hosted by OICcam.com Ideas, requests, problems regarding TWiki? Send feedback. Ask community in the support forum.
Copyright © 1999-2017 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.