Bug - Won't Fix Substantial performance penalty between r12270 and r12271

[PeKaJe]

For a while now I've had to wait upwards of 35 seconds for my equipment inventory tab to load in the relay browser, where it usually takes 7-10 seconds at most (OK, I've got a fairly messy inventory, but still). I finally got curious and started downloading older builds until I found where exactly the big performance hit occurred.

Turns out it is with the change that introduced the "New Area Unlocked" links. To verify, I updated my local repository and commented out "RequestEditorKit.addNewLocationLinks( buffer );", and was immediately back to the old (still slow, but acceptable) performance. I haven't done much digging, but if it does all that pattern matching on a page as big as my inventory (~800kb HTML), I can certainly understand why it's taking much longer than usual.

As nice as those links are, I find it hard to justify the performance hit. Maybe it could be called on a smaller set of pages? And then hope nothing used from inventory unlocks a new zone with the area unlocked message, I guess. Or maybe the regex just needs to be optimized a bit, for instance by having a simpler one to detect the presence of the unlock box, and only then parse with the complex regex that does the additional group capturing.

 

[Veracity]

Ouch.

 

[Veracity]

OK, revision 12295 only adds those links to certain pages:

choice.php (Mr. Alarm)
council.php
forestvillage.php (Untinker)
guild.php
monkeycastle.php (Grandpa)
place.php (Trapper)

Note that the parentheses are examples of page visits that start with that url that can possibly give you New Area Unlocked; I did not make it look ONLY for the Trapper - place.php?whichplace=mclargehuge&action=trappercabin - or Grandpa (just as well, since I believe that both Little Brother and Big Brother can also unlock locations).

Tell me how it performs for you now.

And now we will have new bug reports if I forgot some locations. :-/

 

[lostcalpolydude]

Or KoL will just add new places for it to be needed. Excluding inventory, closet, and storage seems like a decent compromise.

Edit: Or not, now that I see what was done with the code.

 

[Veracity]

And mall searches. And your store inventory. And various other places which can potentially get large.

So, which is better?

- Having to add a new call to addNewLocationLinks in RequestEditorKit.applyPageAdjustments() when KoL adds a new place.
- Calling the decorator for all pages, the vast majority of which will never have a New Area Discovered, and excluding only those which we think can get "big"?

 

[PeKaJe]

OK, well the latest version does fix my performance problem, but maintenance-wise it's maybe not the best solution. I just tried a quick and dirty fix of first testing on the simple pattern "<b>New Area Unlocked</b></td></tr>" (might want to grab a little more, but nothing with wildcards), and if it was found, then run through the existing code. I verified that I had no performance problem on loading inventory, and I got abyss-unlocking linkified (don't have others to test at the moment).

 

[Veracity]

but maintenance-wise it's maybe not the best solution.

You are obviously oh so much more experienced than I am. Thanks for the lecture.

 

[PeKaJe]

Wasn't intended as a lecture. Was just pointing out that between:

- Having to add a new call to addNewLocationLinks in RequestEditorKit.applyPageAdjustments() when KoL adds a new place.

and

- Calling the decorator for all pages, the vast majority of which will never have a New Area Discovered, and excluding only those which we think can get "big"?

there was a third option

- See if the decorator needs to be called, then call it. No need to ever add new calls to applyPageAdjustments.

Sheesh ...