cclaerhout / xen_BBM_v2

Version 2 of the BbCodes and Buttons Manager (BBM) for XenForo (working with TinyMCE 4)
6 stars 1 forks source link

Bbm PreCache System - use bbcode parse cache #18

Closed Xon closed 9 years ago

Xon commented 9 years ago

The Bbm precache system doesn't use XenForo's bbcode parse cache when available. This can make a massive difference for content with significantly complex bbcode.

I'm planning on working on transporting the style of parsing used for the tags map, into the Bbm precache system to determine how much of a cost saving this will be.

Xon commented 9 years ago

https://github.com/Xon/xen_BBM_v2/tree/preCachingTweaks

Testing still in progress, but initial work looks good.

cclaerhout commented 9 years ago

If I remember correctly the precache was only targeting Bb Codes with the PreCache option enabled. So this should be a really few and I'm not sure the performance would improve a lot.

About the modifications you did, there are really a lot. I was already walking on eggs here, so try to be sure it doesn't create other bugs. For example I saw you deleted this line: /Bb Codes MAP/ $this->_resetIncrementation(); And i remember is was needed otherwise the map was not accurate (it counted some Bb Codes twice).

Questions:

Xon commented 9 years ago

So this should be a really few and I'm not sure the performance would improve a lot.

What I observed was the PreCache option was hitting bbcodes which didn't opt-in.

For a problem thread page, I've gone from ~2 seconds for the page down to 1.1 seconds with the optimizations I've done. With a fair chunk still being that the cached parse tree isn't being reused.

My branch doesn't yet fully re-use the available parse tree for some reason. Once that works, I'm expecting a fairly large drop in wall time.

For example I saw you deleted this line:

$this->_resetIncrementation() was moved into setView() and called after the while loop which processed bbcode parse tree chunks.

are you sure the class loader is not needed anymore?

php 5.6 works, but I'll test with 5.3. It isn't a big deal to roll that back.

cclaerhout commented 9 years ago

I've gone from ~2 seconds for the page down to 1.1 seconds with the optimizations I've done.

Ok my bad, that makes a huge improvement, I'm surprised.

$this->_resetIncrementation() was moved into setView()

Thanks ! I didn't see this.

The dice addon will help anyway to check if everything is working as intended :)

Xon commented 9 years ago

Ok my bad, that makes a huge improvement, I'm surprised.

Now that the bbcode parse cache is being effective, the same page is down to ~0.7 seconds.

The dice addon will help anyway to check if everything is working as intended :)

I'm using this addon: Xenforo-ThreadPostBBCode to verify everything is working. The problem thread (well actually problem post), is a single post with ~670 [post] links. The [post] bbcode resolves to the actual thread. Something like: Digital Point [thread] & [post] BBCode except designed to handle users going crazy with them.

Otherwise, hard linking to posts via the /posts/ link doesn't tell users what page some fanfiction update is one, and a /threads/ link will break if the thread is merged, posts deleted or moderator.

cclaerhout commented 9 years ago

Oh I didn't know you coded that, I will take a look ! Thanks :+1:

Xon commented 9 years ago

are you sure the class loader is not needed anymore?

I just tested with php 5.3, and it works.

Xon commented 9 years ago

And i remember is was needed otherwise the map was not accurate (it counted some Bb Codes twice).

The thing I'm worried about is I've forced all non-pre-cached enrolled tags to go through renderInvalidTag, rather than renderValidTag. This might cause the map to be out-of-sync, but it should be doable to fix that.

Xon commented 9 years ago

I have definitely broken something with the Dice bbcode, it throws some nasty to debug exceptions.

Looks like the tag map is out, but this can be reproduced if you delete a bbcode and it is now dealing with an invalid bbcode.

Xon commented 9 years ago

I've identified an underlying bug in BBM. Most recent commit fixes it. Do you want that in a separate pull request or part of the pull-request for this work?

The easiest way to hit the bug in master BBM, is to hit the bb-code nesting limit. This will then cause the tag map to lose track of invalid tags which have tag-map entries.

cclaerhout commented 9 years ago

Thanks for all of this ! I have a quick look, merge your commits and try all this.

cclaerhout commented 9 years ago

It seems there's still a bug (it was there before your modifications). For example adds an img Bb Code in your signature, post several messages, if you enable the display of the tags map, you will see the signature Bb Codes will be processed twice (without causes any problems to the map). I'm going to look at it.

cclaerhout commented 9 years ago

Ok, I found the source of the problem: In BBM_Listeners_AllInOne, this code: $template = $view->createOwnTemplateObject(); Why ? No idea yet, time for me to go to bed now.

cclaerhout commented 9 years ago

ok it's coming from an external addon I installed on my dev board "ControlNoFollow" (I can't remember why I installed it). I will look later why this occurs. Anyway, your mods are all good, nothing breaks. Thanks.

Xon commented 9 years ago

:+1:

Awesome.

cclaerhout commented 9 years ago

I've used another way to get the controller name/action & view name to avoid the problem. I will add the permissions for the XenForo 'color" Bb Code later as it was requested.

Xon commented 9 years ago

19 resolved this