Changed importVariable to store its ownvariables

[flibbles]

More clock-cycles for ya'

The ImportVariable widget creates a setWidget for every single variable it would find in its tiddlers, and it would create a long-ass call trees. Instead, maybe it should just instantiate the SetWidgets just long enough to extract the variable they computed, and then store the variable in itself.

This seems like such an obvious improvement, that I feel like I'm overlooking something. So let me know if you do. Because otherwise, check out these savings:

The following snapshots were created from

1. Opening my tiddler project.
2. Opening and editing one fairly populated tiddler
3. Initiating profiling
4. Saving changes
5. End profiling

This is the performance of vanilla:

Those purple barcode towers go on for several pages. They're tiddlywiki traversing a long tree of setWidgets, one for each globally-defined variable. It amounts to 200-300 functions calls before anything of substance gets called. And if any widget tries to look up a global variable, it must traverse far back up the tree (albeit through prototyping, which isn't too bad performance wise). This traversal happens for every rendered tiddler.

Now this is with my improvement:

Those huge towers are now compressed inside the ImportVariablesWidget.refresh call.

If you look at the total time at the bottom, they are 242 ms and 165ms, which suggests a 30% speed improvement in refreshing tiddlers. I did see a lot of variation, but in average, 80 ms was always shaved off in the performance.

I don't think the improvement is this good. I think it's just Chrome optimizing its profiling for one over the other. That's why one snapshot has clean purple bars, while the other looks like barcodes. But still, I counted, and there are about 4000 less function calls when refreshing a tiddler. So I'm certain there is an improvement.

Also, memory. Since we're not keeping around all those pointless SetWidgets, there should be a theoretical improvement in tiddlywiki's memory foot print. Let's see:

And now:

Okay, so only about 20 KB were shaved off. But it's not nothing.

I've been trying to figure out some circumstance where this improvement causes problems, but I've yet to come up with one. @Jermolene, you know the code base best. Is there a reason we created mock-SetWidgets for every single variable to use on every single tiddler?

[Jermolene]

Thanks @flibbles very cunning, and thanks for the detailed notes. I'm keen to get v5.1.20 released soon, so I'd prefer to leave this until afterwards so that we can thoroughly test it.

I don't think the improvement is this good. I think it's just Chrome optimizing its profiling for one over the other. That's why one snapshot has clean purple bars, while the other looks like barcodes

It's a huge win if it makes browser dev tool flame graphs more usable.

Also, memory

Terrific that you're so adept with the tools. We've not paid much attention to memory usage and I suspect we have some leaks...

[flibbles]

Understood. It'll be here waiting.

(I profiled it on Firefox too. About 400ms -> 320ms, but I still don't think the performance improvement is THAT good)

[flibbles]

@Jermolene Might this come in now that v5.1.20 is out?

If it makes you feel more confident about this change, I've been using it for months now for my own projects, and for my Relink plugin. No problems at all.

[Jermolene]

Thanks @flibbles - there's actually going to be a v5.1.21 bug fix release soon, and then I'll work through the PRs with the tag "post-v5.1.20".

[pmario]

@Jermolene ... Did some debugging and would really love to have this PR merged. Using FF it has 2 nice effects: Makes UI snappier and creates a much nicer flame-chart.

[pmario]

@flibbles ... There seems to be a problem in tiddler edit-mode

[flibbles]

What do you mean?

[pmario]

RSOD: TypeError: eventInfo.handlerObject[eventInfo.handlerMethod] is undefined

[flibbles]

I need a bit more information, because my modification has been working for me.

[flibbles]

I mean, I see where that error must be occurring. I just can't seem to replicate it.

[pmario]

I'm running it with:

node tiddlywiki edtions/tw5.com-server --server

May be a plugin problem somewhere

[Jermolene]

Thanks for the nudge @flibbles, I'd like to get this merged.

@pmario thanks for reviewing it, I'm not going to have a chance to look at it for a few days I'm afraid.

[pmario]

node 12.13.1

[flibbles]

I'm still not replicating it. When you say edit mode, do you mean editing and saving a tiddler?

[pmario]

• create a new tiddler
• click into text area. -> RSOD

[flibbles]

Also, looking at the code, I don't see how this exception could ever be thrown.

exports.addEventListeners = function(domNode,events) {
    $tw.utils.each(events,function(eventInfo) {
        var handler;
        if(eventInfo.handlerFunction) {
            handler = eventInfo.handlerFunction;
        } else if(eventInfo.handlerObject) {
            if(eventInfo.handlerMethod) {
                handler = function(event) {
                    eventInfo.handlerObject[eventInfo.handlerMethod].call(eventInfo.handlerObject,event);
                };	
            } else {
                handler = eventInfo.handlerObject;
            }
        }
        domNode.addEventListener(eventInfo.name,handler,false);
    });
};

It always checks if it's undefined before using it. What version of Tiddlywiki are you using? I'm trying this on cutting edge right now. I've been using it on v20 I think.

[flibbles]

Oops. Nevermind. I'm stupid. It can be thrown (although it's still not happening for me).

[pmario]

I did use your "enhence" branch. Maybe I should merge it to master.

[flibbles]

Theeeerrrre it is. Yeah. I'm getting it when I'm strictly on my enhance branch. Not sure why it's broken. It works fine when merged, but I'll look into this. I should at least know why this is happening.

[pmario]

seems to be my fault :/ ... if merged with master it seems to work

[flibbles]

Hmm. Okay. Looks like when I made the PR, I rebased onto master at a point when an unrelated bug was checked in. My bad. Merging it does indeed resolve the problem.

Should be good for merging. I've been using this modification for several months now without issue.

[Jermolene]

Hi @flibbles I think this change may not be entirely backwards compatible, albeit subtly.

Importvariables will extract any set widgets that it finds. It doesn't care whether those set widgets were created with \define or via an explicit <$set>. In the latter case, it is possible for importvariables to bring in something like <$set name=myvar value={{mytiddler}}/>. The trouble is that because the set widget is removed from the widget tree there's no way for the variable value to get refreshed when mytiddler changes.

Using <$set> widgets in that way would be unusual but it works today. I'll give it a bit more thought; this may be a case where tactically breaking somewhat theoretical backwards compatibility is worth it.

[pmario]

@Jermolene I did the following test, which works as expected.

test-fibbles.zip

the zip contains 3 tiddlers.

test-macro

\define test() {{my-tiddler}}

<pre><$view field="text"/></pre>

result

<<test>>

my-tiddler

aaaaaaaaaa

I do open the dev console and write.

var x = $tw.wiki.getTiddler("my-tiddler")
$tw.wiki.addTiddler(new $tw.Tiddler(x,{text: "test test test"}))

And everything updates as expected.

[Jermolene]

@pmario you need to use a <$set> widget, not a \define, otherwise the transclusion won't be processed.

[pmario]

How? A definition like the following won't be imported by importvariables. It only gets macro definitions.

<$set name="myVariable" value={{my-tiddler}}></$set>

[pmario]

Importvariables will extract any set widgets that it finds. It doesn't care whether those set widgets were created with \define or via an explicit <$set>.

I think that's not true. At least I'm unable to create variables that are imported like this.

[flibbles]

Okay, I see what you mean, @Jermolene. The following reveals the issue

title: val

AAA

title: mac

<$set name="mymacro" value={{val}} >

<<mymacro>>
</$set>

title: test

\import mac

<<mymacro>>

Then you change the value of val from AAA to BBB. tiddler mac shows the update properly, but test does not.

Hmm. I guess you're right. The old way wouldn't have had this problem. Though I'm also inclined to agree with @pmario. I was very surprised when working on this issue to find out that <$set> widgets are pulled in by importVariable. Their very syntax implies that they should be self-contained to whatever is between their elements.

Ever since I've found out, I've been concerned that setWidgets are polluting my global macro namespace, and I wouldn't be surprised if very few, if any, deliberately use <$set> to define global macros.

[pmario]

@flibbles Did you test your example with tiddlywiki.com? It doesn't work for me, if I use

<$set name="mymacro" value={{val}} >
<<mymacro>>
</$set>

The whitespace is needed. Why. IMO that's a bug in tiddlywiki.com?

[flibbles]

I'm using my enhance branch merged onto v5.1.21. I tried it on tw5.com-server and was able to replicate the issue using the tiddler examples I just provided.

The whitespace shouldn't be needed. I just always prefer block rendering to inline.

[pmario]

Jeremy wrote

Using <$set> widgets in that way would be unusual but it works today. I'll give it a bit more thought; this may be a case where tactically breaking somewhat theoretical backwards compatibility is worth it.

hmmm, I couldn't even think about an implementation that is different between "old" and "new".

I think most users use the $:/tags/Macros definition to create global macros. The tiddler code would look like this: <$set name="mymacro" value={{val}}/>

Which doesn't work with tiddlywiki.com

[pmario]

2 years ago parameter substitution for set-variables was suppressed. See: 35cbb12

This created a difference between macro definitions and set-variables since version 5.1.18 (I think). So was it an oversight, that import-variables didn't make a difference here?

[flibbles]

If you really want to know why importing <$set> widgets is weird, feast your eyes:

title: macros

<$set name="outer" value="Cats">

   <$set name="inner" value="<<outer>>" >

   </$set>
</$set>

<$set name="outer" value="Dogs">

</$set>

title: test

\import macros

<<inner>>

One would think macro inner would only use the macro outer that contains it, but in this case, you can get the inner macro to render using an unrelated outer macro. It's not intuitive.

[pmario]

@flibbles ... Yea, I think that your PR makes things more consistent and intuitive. So only \defined set-variables should work.

I think, this would also make it possible to speed up the while loop, if we would check for isMacroDefinition. Because if it is not, it could be ignored. ... just a thought.

[flibbles]

Actually, what I just posted doesn't work. My bad, because only the first dive into setWidgets is read.

<$set A><$set B><$set C></$set></$set></$set>
<$set D></$set>

D gets missed. And yeah @pmario. I would think it's more intuitive if non-isMacroDefinition weren't included during imports.

[Jermolene]

The fact that importvariables works with explicit set widgets is really a feature, not a bug. It means for example that it's easy to have a global variable that is dynamically recomputed. In fact, thinking about it, we could make the main page template a lot more extensible by implementing the current nested set widgets to a single tagged importvariables.

[flibbles]

In that case, there's no optimization to be had. Not unless one can think of a way to store the necessary components in ImportVariables so that it performs execution with everything else instead of doing it before hand. But that may involve storing so much stuff in the importVariables widget that much of the benefit is lost.

Pity. The way ImportVariables works right now, Tiddlywiki's stack depth increases with every globally defined macro.

[flibbles]

Well... maybe there is a way. If ImportVariables holds onto its own tree of setWidgets, but it doesn't actually add them as children, it can still recalculate them as needed, and use them to replace the variables within itself. Maybe? I think this might work?

However, this does cost us some of the optimization's benefits, and all of the optimizations memory improvement.

I can try this, @Jermolene, if you're adamant that <$set> widgets must remain supported, though I still find it odd that importing the tiddler:

<$set name="A" value="value1">text</$set>
<$set name="B" value="value2">text</$set>

will import A, but not B.

[Jermolene]

@flibbles perhaps we could use the isMacroDefinition flag, and thus only process the <$set> widgets that are "safe", leaving the others intact?

[flibbles]

There is merit to that. I'd have to be careful though. Implementing /imports and <$sets> using different systems might result in incorrect behavior in cases where variables are defined multiple times.

I think whatever I do, I'll need to pin down the behavior with a few unit tests that we can all agree on.

[flibbles]

Okay, @Jermolene, I've followed your advice. New commits added. Whenever ImportVariables imports a setWidget, it adds it onto the end of its chain like it used to. But whenever it imports a macro definition, it folds the variable into whatever widget is on the end of the chain.

Since >99% of all imported variables are macros, that means we get the performance boost of my improvement while still being able to handle setWidgets with non-simple attributes.

Also wrote a couple tests.

[flibbles]

@Jermolene, should be good to go, unless there are any other objections.

[flibbles]

bump

[flibbles]

@Jermolene nudge

[Jermolene]

Thanks @flibbles