search

TiddlyWiki / TiddlyWiki5

A self-contained JavaScript wiki for the browser, Node.js, AWS Lambda etc.
https://tiddlywiki.com/
Other
7.98k stars 1.18k forks source link

About existing TOC vs. prerelease TOC performance degrading #3517

Closed pmario closed 5 years ago

pmario commented 5 years ago

I did some measurements with tiddlywiki.com and tiddlywiki.com/prerelease

The following code was measured, which renders the full structure. 

<div class="tc-table-of-contents">
<<toc "TableOfContents">>
</div>
TW from to
tiddlywiki.com 1600ms 1900ms
prerelease 2300ms 2900ms

IMO That's way too much!

My system:

Jermolene commented 5 years ago

Thanks @pmario that's helpful. We should definitely aim to improve performance in cases like this.

pmario commented 5 years ago

There have been some heavy improvements, to make the code more stable against "nasty titles". ... It can happen :)

pmario commented 5 years ago

New test with a slightly modified version. All paths expanded and DOM structure retained.

Steps to prepare measurement: 

<div class="tc-table-of-contents">
<<toc-selective-expandable "TableOfContents">>
</div>
TW Version from to
V 5.1.17 3700ms 3990ms
prerelease latest 5000 ms 5400 ms
commit 587fe9d10 4300 ms 4800 ms

more to follow

pmario commented 5 years ago

@Jermolene commit 587fe9d10 which contains wikify seems to be faster as the latest master branch :/ ... That's surprising

pmario commented 5 years ago

zip file, that contains all 3 versions from above. Downloads.zip

Jermolene commented 5 years ago

Hi @pmario I've been unable to duplicate these findings.

As a test rig, I turned on performance instrumentation, and replaced the inner list filter of $:/core/ui/PageTemplate with:

<div class="tc-table-of-contents">

<<toc "TableOfContents">>

</div>

Then I compared performance with the current version of toc.tid with the v5.1.17 one. On my MacBook Pro I get virtually identical readings for both versions of the code: around 3s for the initial main render, and 0.6s for the initial main refresh.

pmario commented 5 years ago

@Jermolene ... The toc macro doesn't use the reveal widgets. .. So there is basically no difference in timing. ... as you found out.

the proper test is: https://github.com/Jermolene/TiddlyWiki5/issues/3517#issuecomment-436626603

The Downloads.zip file contains all 3 versions, that I used for testing. You could use them.

pmario commented 5 years ago

retain=yes ... imo is not needed for the test.

The recursion error was eliminated some time ago.

The "kin" filter may be able to help with multiple tags in the TOC, because it can eliminate dups ... which will lead to "strange" behaviour for some users. ...

The surprise for me was that the "wikify-version" was faster that the latest iteration. ... It seems to have a "caching" effect.

pmario commented 5 years ago

To be honest. I could live with commit 587fe9d10

I was thinking about some "hardcoded" JS-toc-macros, which should be able to speed things up, quite a bit. ... may be :)

pmario commented 5 years ago

just to be sure. ... The reveal default=open retain=yes where only made for testing, to get the whole tree expanded by default.

Jermolene commented 5 years ago

just to be sure. ... The reveal default=open retain=yes where only made for testing, to get the whole tree expanded by default.

Indeed I misread your message in haste, and deleted my subsequent comment.

Is 587fe9d10eaa1e837da02f3d8c5f885acef9da10 the commit where the performance regression was introduced?

Jermolene commented 5 years ago

Is 587fe9d the commit where the performance regression was introduced?

Just realised that that commit doesn't reflect the current code; we got rid of the wikify call since then.

This URL shows the changes to the TOC macro:

https://github.com/Jermolene/TiddlyWiki5/compare/v5.1.17...master#diff-76542ab9902926648b40dfa8e112bcb6

BurningTreeC commented 5 years ago

@pmario, @Jermolene

the toc macros use the stateTitle attributes now

there's this line in the reveal widget:

https://github.com/Jermolene/TiddlyWiki5/blob/018f7628f565269395950a0b94af7eadd05484c9/core/modules/widgets/reveal.js#L124

Could this be part of the slow-down?

BurningTreeC commented 5 years ago

To above: I mean line 124 and the preceding lines

Jermolene commented 5 years ago

Hi @BurningTreeC I'm not a fan of combining conditionals like that, but I don't think it's problematic from a performance perspective. I'm wondering if it's the switch from text substitution to a lot more filtered transcluded attributes (ie, attr={{{ blah }}}).

BurningTreeC commented 5 years ago

@Jermolene, do we have an alternative for {{{ blah }}}?

BurningTreeC commented 5 years ago

if we replace all {{{ blah }}} through wikify widgets that do:

\define toc-add-suffix(prefix,suffix)
$prefix$$suffix$
\end
<$wikify name="path" text="""<$macrocall $name="toc-add-suffix" prefix=<<__path__>> suffix=<<__tag__>>>
Jermolene commented 5 years ago

I would expect a wikify widget to be a lot slower than a filter. The wikify widget parses it's text, and renders it to a widget tree and fake DOM before extracting the resulting text. The reason that I didn't implement the wikify widget for a long time was that I thought it absurdly inefficient.

Meanwhile, filter parsing and evaluation is heavily optimised, and so I'd expect it to be a lot faster.

pmario commented 5 years ago

Is 587fe9d the commit where the performance regression was introduced?

It was the first one, where I did measurements. ... I had concerns about the performance, that wikify may have introduced. ...

As it turns out, removing wikify didn't fix it, but made it worse.

pmario commented 5 years ago

The first changes, that where done was to go from """$param""" -> <<__param__>> ... which is still part of the code. (for a reason)

pmario commented 5 years ago

The toc macros are "heavily" recursive. It's expected to be expensive to be calculated. ...

My goal is to have UI feedback to be (way) below 400ms. ... That's my personal preference.

BurningTreeC commented 5 years ago

To my idea above using the wikify widget, very bad idea, I tried -> more than 7 seconds

The first changes, that where done was to go from """$param""" -> <<__param__>> ... which is still part of the code. (for a reason)

yes, we wanted to make it more robust for titles with quotes etc. which worked

@pmario do you think replacing the wikify widget that created the qualified state with the <$qualify> widget made it worse?

pmario commented 5 years ago

@pmario do you think replacing the wikify widget that created the qualified state with the <$qualify> widget made it worse?

The funny thing is. wikify wasn't the slowest solution.

If you have a look at the download.zip file from https://github.com/Jermolene/TiddlyWiki5/issues/3517#issuecomment-436629312 you can see the differences with F12 console. If you switch between toc-tab and an other tab.

BurningTreeC commented 5 years ago

do you think replacing the wikify widget that created the qualified state with the <$qualify> widget made it worse?

I tried, it's not the case

BurningTreeC commented 5 years ago

If you have a look at the download.zip file from #3517 (comment) you can see the differences with F12 console. If you switch between toc-tab and an other tab.

Yeah I see, I'm just doing that right now

pmario commented 5 years ago

open and close "New Tiddler" is also possible

BurningTreeC commented 5 years ago

I've cloned the current prerelease, set every reveal widget in the TOC macro to open and retain to yes and enabled performance instrumentation. Testing with the latest tw5.com

pmario commented 5 years ago

..Testing with the latest tw5.com

I didn't test the latest version yet.

pmario commented 5 years ago

latest master FF64:

performance: styleRefresh: 3.00ms performance: +filter: 0.00ms performance: mainRefresh: 4912.00ms performance: +filter: 687.00ms

MS Edge 17.17134 / win 10

performance: styleRefresh: 14.60ms performance: +filter: 3.40ms performance: mainRefresh: 11772.50ms performance: +filter: 2552.00ms

BurningTreeC commented 5 years ago

I've merged the kin-filter pr locally into the current master, enabled performance instrumentation and changed the toc-macro reveal widgets to open and retained. there are two toc sidebar tabs, one the normal toc, one filtered by the kin filter

created a single html from it:

tw5118kin.zip

BurningTreeC commented 5 years ago

another thing surely hitting performance is the amount of tiddlers

5.1.17 gives me 2908 tiddlers

5.1.18 gives me 3311

just counting them through

{{{ [all[tiddlers+shadows]count[]] }}}
BurningTreeC commented 5 years ago
Jermolene commented 5 years ago

@BurningTreeC can you confirm you're seeing the same performance degradation reported by @pmario?

If so, does using the toc.tid from 5.1.17 with the latest prerelease take us back to 5.1.17 performance?

BurningTreeC commented 5 years ago

Hi @Jermolene , what Ican confirm is that there's a difference in rendering time like @pmario describes when using the latest Firefox nightly on Linux (with its new webrenderer on)

BurningTreeC commented 5 years ago

I haven't tested on more browsers and platforms, just couldn't find the time

BurningTreeC commented 5 years ago

What I propose is that we give 5.1.18 a go and let this out to more systems and browsers. I think that working and playing with it we'll get more ideas

BurningTreeC commented 5 years ago

We could also take the old toc- macros back in and name the new ones toc-solid, toc-selective-expandable-solid ...

BurningTreeC commented 5 years ago

So for 5.1.18 this could be a temporary solution

BurningTreeC commented 5 years ago

3621 has the new toc macros renamed so that they all end with -solid and put the 5.1.17 macros back in with their original names

BurningTreeC commented 5 years ago

I found an open macrocall in the toc macros and fixed it in #3622 for the new macros and in #3621, too

Jermolene commented 5 years ago

Hi @BurningTreeC I think I'd rather go with the current version, improve it, or pull the changes entirely. #3621 could be a useful basis for a performance test rig though...

BurningTreeC commented 5 years ago

Yes, @Jermolene ... I'd do the same

BurningTreeC commented 5 years ago

I can just say that now testing on chrome and firefox, the results do vary a bit. we need more experience with this and should just go playing around :grin:

BurningTreeC commented 5 years ago

To your question above, yes, the 5.1.17 macros are more performant in 5.1.18, too

But I believe the new ones are how they should work

So I don't know and decision is up to you

BurningTreeC commented 5 years ago

For 5.1.18 let's launch a contest in the Google group who comes up with the most performant TOC macro that handles titles well

Jermolene commented 5 years ago

I've been experimenting with a slightly different setup which you can see in the branch "toc-performance" - https://github.com/Jermolene/TiddlyWiki5/tree/toc-performance

As presently configured, the v5.1.17 macros are used (with the mods suggested by @pmario above). To test it with the v5.1.18 macros, change the tag on toc.tid to be $:/tags/Macro" and change the tag on toc-5.1.17.tid to be$:/tags/Macro2`.

In my tests on macOS Chrome, the main render times for 10 consecutive runs with the v5.1.17 TOC macro were (to 2 significant figures):

The same timings for the v5.1.18 TOC macros were:

So, on that basis, I'm going to conclude that @pmario's results were due to the test methodology, and proceed with the v5.1.18 release.