Maptool: Improve Macro performance

[nmeier]

Describe the bug

Macros have a couple of areas where they could be improved

1. low level usage of lesser than optimal APIs (e.g. trim() where isBlank can be used) creating more temporary Strings - while that's not a big deal in modern Java, repeated ops like that can slow things down

2. Parser/Expression re-creation on every macro line, even in a loop. One could re-use ExpressionParser/Expressions

3. slower than optimal String functions as every string operation causes the full list to be parsed and converted into ArrayList first, Cost: more String instantiations (minor) but more importantly doesn't allow for optimization (e.g. listGet(0) which should be O(1) but is O(n))

4. Even if a script uses GUIDs (getTokens() rather than getTokenNames() the lookup in Zone.resolveToken() does an expensive case insensitive check for all token names first, before trying to interpret as GUID. That can be made faster.

5. json Arrays are flattened/reparsed unnecessarily between parser lines (in both maptool and parser)

To Reproduce

Hard to measure. I've run performance tests at scale (over 500 tokens, with 500 elements) to work around variances in function execution. My test macros (will attach) for sufficiently large sets of tokens to iterate over improved by 3-5x.

I've also run specific function tests at scale which are 10% faster for worst-case and 5-10x faster for delete(), find(), get(), append() for optimal indexes/simple appending.

Expected behavior

Faster macros

Screenshots

n/a

MapTool Info

• Version: 1.7
• Install: dev

Desktop (please complete the following information):

• OS: Windows

Additional context

Adding macro examples with PR. I'll prepare PRs for different areas 1-4

[Merudo]

I'm not sure if I fully understand what you mean by 3a and 3b (I'll have to look again at the code), but I think 1, 2, and 4 should definitely be implemented.

[cwisniew]

I think 3a/3b boil down to don't use String lists :)
The only internal representation for them is Strings.
Even though json has a parsing cost for each fetch from a property its at least a one off, rather than the pathological case for looping through a string list.

I have plans for notebooks to reduce that parsing for json to once per campaign, actually we could probably do that for token properties too. But I have no such plans for String lists because well people expect them to be Strings so how else would you store them?

[nmeier]

Tried to clarify 3a a bit - if you call stringGet(i) for all elements in a list, which is a string with delimiter in a macro, the string is parsed n times into a List to then do a O(1) access for that element.

[nmeier]

3b is about the care that maptool uses to return pretty strings - all string list functions return

a, b, c, d, e

that's not technically necessary as all string list functions can operate on anything like

a , b, c , d , e

If that pretty formatting is not a requirement you get better performing calls on average (worst case is still bad as these strings are never lists between macro invocations)

[cwisniew]

Tried to clarify 3a a bit - if you call stringGet(i) for all elements in a list, which is a string with delimiter in a macro, the string is parsed n times into a List to then do a O(1) access for that element.

This I understand, but each call to stringGet(index) is a different call in a loop, we don't actually have a parser there (don't let the name MapToolLineParser fool you), if it was a proper parser that built an AST or equivalent it would be trivial, but getting index 2 has no way of knowing that you previously got index one and no real knowledge its in a loop. With JSON it's easier as its not expected to be a string so we can store it in MapToolVariableResolver as a JsonObject or JsonArray. You can do similar with string list / prop as they should always be useable as strings. So strList1 + ", " + strList2 should work and that + is handled by the actual parser which knows nothing about anything not a number or string...

3b is about the care that maptool uses to return pretty strings - all string list functions return

a, b, c, d, e

that's not technically necessary as all string list functions can operate on anything like

a , b, c , d , e

If that pretty formatting is not a requirement you get better performing calls on average (worst case is still bad as these strings are never lists between macro invocations)

I don't think you can change this at this point, people expect it the way it is, trust me there have been bugs filed when new functions return something slightly different space wise.

[Merudo]

Even though json has a parsing cost for each fetch from a property its at least a one off, rather than the pathological case for looping through a string list.

So looping through a json array is faster than looping through a string list? All this time I used string lists for collections of token ids, thinking looping through them would be faster than with a json array.

[cwisniew]

Even though json has a parsing cost for each fetch from a property its at least a one off, rather than the pathological case for looping through a string list.

So looping through a json array is faster than looping through a string list? All this time I used string list for collections of token ids, thinking looping through them would be faster than with a json array.

Yes, but...
Storing a JsonArray in a token property requires serializing to String, which means reading back from a token property requires parsing it again. Thats the major slow down with JSON. If it's in a variable its already stored as a JsonObject or JsonArray and there is no serialization / parsing required, and its performance in a loop is similar to a java ArrayList (its backed by one) with some little overhead for type conversion (but thats negligible).

The reason its a String in the Token Properties is because the UI only understands Strings and Numbers, and also to maintain save compatibility. One alternative is to create a transient Map with the JsonObject / JsonArray, populating on first parse, and populating on put (if its a JsonObject/JsonArray) and the serialized version in the "saved" property map. That way at least its only parsed a single time per campaign load.

[nmeier]

This I understand, but each call to stringGet(index) is a different call in a loop

Two examples where improvement can be improved

1. getting the first element of a list (pop) can be done without looping through the whole list

listget("99,98,97,...", 0) == 99

1. looping through a list in a macro

listget("99,98,97,...", i)
instead of nn constructions of an ArrayList you loop through the list N(N+1)/2. For a list with 100 entries that's 10k elements parsed currently, it can be 5k elements parses (1,2,3,4,5,6,...).

With JSON it's easier as its not expected to be a string so we can store it in MapToolVariableResolver as a JsonObject or JsonArray.

3b is about the care that maptool uses to return pretty strings - all string list functions return
I don't think you can change this at this point, people expect it the way it is, trust me there have been bugs filed when new functions return something slightly different space wise.

That would be unfortunate. Do you have an example bug that reports this? Would like to understand what the use case is. I assume everyone is parsing for commas. Expecting exact comma+space seems to indicate another problem.

[nmeier]

json functions do offer the faster operation so of course that could be the right answer for some of the concerns on stringfunctions being slow that's true

[nmeier]

So looping through a json array is faster than looping through a string list?
Yes, but...

Hmm, I'm not seeing the 'faster' yet - am I doing something wrong in this macro? Want to go through all tokens on all maps and build a list of pairs.

[h: maps = getAllMapNames("json")]

[h: maptokens = '[]']
[h, FOREACH(map, maps), code:{
    [h: tokenids = getTokens(",", json.set("{}", "layer", json.append("[]","TOKEN"), "mapName", map ))] 
    [h, FOREACH(tokenid, tokenids, ","): maptokens = json.append(maptokens, map, tokenid)]
}]

It seems that the json array is converted back and forth between string and internal jason representation (through coerceToJsonArray and when the deterministic parser is created).

[nmeier]

pull request is only for token lookup - 4.

[JamzTheMan]

This I understand, but each call to stringGet(index) is a different call in a loop

Two examples where improvement can be improved

1. getting the first element of a list (pop) can be done without looping through the whole list

listget("99,98,97,...", 0) == 99

1. looping through a list in a macro

listget("99,98,97,...", i)
instead of n_n constructions of an ArrayList you loop through the list N_(N+1)/2. For a list with 100 entries that's 10k elements parsed currently, it can be 5k elements parses (1,2,3,4,5,6,...).

With JSON it's easier as its not expected to be a string so we can store it in MapToolVariableResolver as a JsonObject or JsonArray.

3b is about the care that maptool uses to return pretty strings - all string list functions return
I don't think you can change this at this point, people expect it the way it is, trust me there have been bugs filed when new functions return something slightly different space wise.

That would be unfortunate. Do you have an example bug that reports this? Would like to understand what the use case is. I assume everyone is parsing for commas. Expecting exact comma+space seems to indicate another problem.

I have no use cases so just theorizing here... for processing it shouldn't be a problem, but what about just printing out the list, say, on a character sheet or something? having the extra space would look ugly and unexpected (yet work).

Also, can't string lists have other delimiters? Can't they be defined in for loops like ", "? Not saying you should or people have.

And I don't think we're saying adding a space to the delimiter would be hard to fix, but what if it breaks a campaign framework that the original author no longer supports?

[Azhrei]

That would be unfortunate. Do you have an example bug that reports this? Would like to understand what the use case is. I assume everyone is parsing for commas. Expecting exact comma+space seems to indicate another problem.

I have no use cases so just theorizing here... for processing it shouldn't be a problem, but what about just printing out the list, say, on a character sheet or something? having the extra space would look ugly and unexpected (yet work).

I do this for some of my stat sheet properties. When I import HeroLab data, I add certain strings to a string list and then jump dump the result into a property, knowing that the stat sheet will display it with comma-space between elements.

Otherwise, I agree — there's no need for extra spaces, but this type of formatting is _very_ convenient in actual use.

Also, can't string lists have other delimiters? Can't they be defined in for loops like ", "? Not saying you should or people have.

They can, and I know I've used semicolons at some point, but I don't remember where or why.

[nmeier]

We can certainly keep the spaces in place, that's negligible cost.

If you create a list and append, etc.

"a" + "b" = "a, b"
"a, b" + "c" = "a, b. c"
"a, b, c" replace #1 with x = "a, x, c"

that's doable while making it faster. What's "too much promise" imo is to say

"a,b, c, d" + "c" = "a, b. c, d"
If the input to str functions is not formatted pretty, then I'd argue one could relax the outcome.

"a,b, c, d" + "c" = "a,b, c, d, c"
if all string functions honor the delim+space format then the input list should be well formatted.

input: well formatted
output: well formatted

input: not well formatted
output: only well formatted for that element that was deleted, replaced, appended

still putting that CR together to have some eyes on it. Would be cool if you could check that with your macros for statsheets.

[cwisniew]

So looping through a json array is faster than looping through a string list?
Yes, but...

Hmm, I'm not seeing the 'faster' yet - am I doing something wrong in this macro? Want to go through all tokens on all maps and build a list of pairs.

[h: maps = getAllMapNames("json")]

[h: maptokens = '[]']
[h, FOREACH(map, maps), code:{
  [h: tokenids = getTokens(",", json.set("{}", "layer", json.append("[]","TOKEN"), "mapName", map ))] 
  [h, FOREACH(tokenid, tokenids, ","): maptokens = json.append(maptokens, map, tokenid)]
}]

It seems that the json array is converted back and forth between string and internal jason representation (through coerceToJsonArray and when the deterministic parser is created).

Well this is something that needs to be fixed then :( the whole point of having a json array was something that doesn't need to be a string so shouldnt need to be continually parsed unlike string lists which have to be strings.

[cwisniew]

I am wondering if it might be better to add new type to MapToolVariable resolver and add some indirection.
Something like (not full thing obviously :) )

public enum VariableType {
    STRING_LIST,
    STRING_PROP,
    JSON_ARRAY,
    JSON_OBJECT
}

public class VariableReference {
    private final String stringValue;
    private final UUID referenceId = UUID.randomId();
    private final Object parsedValue;
    private final VariableType type;

    public static VariableReference createJsonReference(String val) {
        // ... 
    }

    public static VariableReference createStringListReference(String val) {
        // ... 
    }

    public static VariableReference createStringPropReference(String val) {
        // ... 
    }

    public String toString() {
        return "@ref:" + referenceId;
    // ...
}

This will require a new Map in the MapToolVariable resolver
valueReferenceMap. (make it a WeakHashMap) to map String variable values (not variable names) to save on reparsing.
The reason for this is things will still come in as strings at times because something has changed them to a string (like storing / retrieving from token).

MapToolVariableResolver.setVariable

1. if type in Json or String Checks to see if a VariableReference already exists for that String, of so sets new value to that reference.
2. Otherwise if Json creates a new reference.
3. Otherwise store as what ever it is (as currently happens)

Also any token values etc set in the method should stay as is.

MapToolVariableResolver.getVariable

1. If the variable is a reference it returns parsedValue if type is Json Array or JsonObject
2. Otherwise it returns the string value.

This will maintain compatibility with current macros.

New method Optional getVariableReference(name)
Returns a variable reference if it exists.

New method Optional getVariableReference(name, type)
Returns a variable reference if it exists, null otherwise, if not will try create a reference based on value in the named variable empty optional if it cannot. If it can create the reference it will replace the variable with the new reference (and update valueReferenceMap).

Macro Function/Loop Changes
Then we can start replacing some calls of getVariable with getVariableReference(), like the string list/prop functions, functions that return a string list or json and loops for starters... For/while loops should then be changed to accept references (it might still have to be a string but I can't currently check the code to make sure, even still parsing that reference string is much quicker than parsing a large Json/StrList/StrProp, also literal strings would need to be turned into a temporary reference which is no biggie.)

Although changing all those functions sounds like a big change its at least something that can be split up so ones with most benefit first. Also I am sure there are others willing to help convert the macro functions.

It's not perfect, I can certainly think of better solutions if anyone wants to actually look at the antlr parser (I am happy to help there but I already have enough in my back log).

[nmeier]

It seems that the json array is converted back and forth between string and internal jason representation (through coerceToJsonArray and when the deterministic parser is created).
Well this is something that needs to be fixed then :( the whole point of having a json array was something that doesn't need to be a string so shouldnt need to be continually parsed unlike string lists which have to be strings.

I'm debugging it. During the AST handling and building a deterministic parser (still not 100% what that is for, possibly for pretty printing the expression like a roll) it seems that the list gets converted to the string.

Agree that JSON objects once stuck into the resolver should simply give you the fastest experience.

[nmeier]

Created a PR to see how optimization for str functions can work. Note the discussion on the pretty formatting.

Pass a pretty formatted string to one of the strfunctions? Get a pretty formatted string back.
Pass a non-pretty format to the strfunction? No guarantee that the whole list is reformatted.

[cwisniew]

I much prefer something like what I have outlined above. As I don't want to just make string lists faster then tell people to use string lists. Especially with the html5 JavaScript additions json should always be preferred

[nmeier]

I don't want to just make string lists faster then tell people to use string lists. Especially with the html5 JavaScript additions json should always be preferred

why not make string lists faster? Afaiks the json optimization is broken deep down in the parser. It's a tad more complicated to do. If string lists are offered as they are right now, I would not keep them nerfed. Any language can append a string to a comma separated pseudo list in one step - and append(), pop() are very basic functions that should just work fast imo.

I much prefer something like what I have outlined above. As

I'd have to see more analysis on how more build up on the existing resolver model improves things underneath. Suggest to fix what's obvious, then think about bigger changes. Ultimately the whole concept of parsing line by line and starting a parser for each expression inside seems reversed to me.

[Azhrei]

Suggest to fix what's obvious, then think about bigger changes.

The whole thing is (eventually) going to be replaced with a real grammar-based approach, so I don't know if it's worthwhile to spend _a lot_ of time on this. However, it's hard to argue with anything that speeds up macros without breaking existing code if it doesn't add to the maintenance cost.

[Merudo]

My understanding is that the real grammar-based approach won't be ported back to MTscript v1, and will only apply to MTscript v2.

Hence, performance improvements to MTscript v1 will remain valuable when working with older macros.

[cwisniew]

I don't want to just make string lists faster then tell people to use string lists. Especially with the html5 JavaScript additions json should always be preferred

why not make string lists faster? Afaiks the json optimization is broken deep down in the parser. It's a tad more complicated to do. If string lists are offered as they are right now, I would not keep them nerfed. Any language can append a string to a comma separated pseudo list in one step - and append(), pop() are very basic functions that should just work fast imo.

I much prefer something like what I have outlined above. As

I'd have to see more analysis on how more build up on the existing resolver model improves things underneath. Suggest to fix what's obvious, then think about bigger changes.

As I have stated its better to have all three problems fixed than just String Lists. And the reason not to do just string lists is because much of the change is not compatible with the change I outlined above -- or at least makes it more difficult. Which leaves three choices

1. Don't put in the string list only fix and do the fix for all three.
2. Put in the string list fix and then when the other fix is done don't apply it to string lists meaning they are not as fast as they could be.
3. Back out the string list only fix to apply the fix to do all three.

Since a lot of my MapTool coding time late last year early this year has been spent going back and redoing partial fixes I am not interested in option 3 as I also have a list of things that I would like to get done. I also don't like option 2 as then I feel like I have to go do 3.

Ultimately the whole concept of parsing line by line and starting a parser for each expression inside seems reversed to me.

You wont get an argument from me there, I wasn't involved in the loops and the first time I saw them I wanted to replace it with an actual parser but was told no. By the time the original mainter left it had grown so complicated and ugly and too many macros out there that any attempt to rewrite it resulted in breaking a lot of them

[nmeier]

reason not to do just string lists is because much of the change is not compatible with the change I outlined above

Don't have too much time unfortunately. I see the concept of adding references in var maps as you suggested to add a lot more complexity. What's the projected benefit of another layer/concept? Is it even faster? Maybe. Looks more complicated and definitely more work though.

Also if it doesn't help any existing macros as mentioned further above that's a minus for me.

I don't know the effort, feasibility and runtime benefit of the bigger proposal over fixing what is.

BTW I figured out why the json arrays are flattened in parser and think I can fix it. It's a 50% difference (only) in runtime. This macro takes 10s in current MapTool. 5s after my change.

[h: list = "[]"]
[h: list = json.append(list, "1", "2", "3") ]
A json list not expanded stays a variable reference until evaluated: [list]
<br>
[h: list = "[]"]
[h, count(10000): list = json.append(list, "x") ]
Repeating json.append for [r: json.length(list)] times without flattening toString() saves time

[nmeier]

Oh, I also just have to package up the change that makes parser/expressions reusable instead of having them re-created for every macro line. That addresses all the performance problems laid out in #1898 for me.

[Merudo]

Exciting stuff! Looking forward to these impressive performance improvements!

[cwisniew]

reason not to do just string lists is because much of the change is not compatible with the change I outlined above

Don't have too much time unfortunately. I see the concept of adding references in var maps as you suggested to add a lot more complexity. What's the projected benefit of another layer/concept? Is it even faster? Maybe. Looks more complicated and definitely more work though.

It turns all String List functions into java list operations (might require a copy of list as they are immutable) + format as string. For operations that don't alter the list and return a scalar like listGet() (probably most common call), listCount(), listContains() it's a huge win. For most others It's still quite a bit faster than what is there today. It can even benefit lists of lists (I don't know why people do that but they do e.g. 1 ; 2 ; 3 , 1 ; 4 ; 7 , 8 ; 9 ; 2) if the inner lists are accessed often...

String Properties and JSON Arrays / Objects are similar.

Also if it doesn't help any existing macros as mentioned further above that's a minus for me.

All benefits apply to existing macros using these, unlike the string list only change.

[nmeier]

Also if it doesn't help any existing macros as mentioned further above that's a minus for me.
All benefits apply to existing macros using these, unlike the string list only change.

What are the issues that need to be addressed in non-string-list macros again?

String-lists can be faster. Jason usage can be faster. Each can be improved independently. Unifying everything under the hood could be nice, but I don't get the advantage of unifying, when both current implementations are not working fast as one would expect.
I'm probably missing context and don't know how far out your change is.

Anyhow, I'll move this PR to a new branch/PR as a possible backup. If someone wants to address the slow performance in strings, that PR can still be pulled.

Are you guys interested in fixing the slow performance of json arrays due to being serialized into strings in parser? Or does that also fall under optimizes-one-but-not-the-other, so let's park?

[cwisniew]

@nmeier Can you please resubmit the PR git hub is being a bit touchy about trying to update it.

@Phergus this is ok to go through, we are going to take a different tack on performance that wont be applied to string lists.

[Phergus]

@cwisniew Roger that.

[Phergus]

New Dicelib release 1.6.3 to enable the use of a non-deterministic parser.
https://github.com/RPTools/dicelib/releases/tag/1.6.3

[Phergus]

Some testing shows that under Java 14, with the previous changes disabled, the code like that above already ran faster.

6 Runs in 1.7.0 averaged: 11.025 secs.
6 Runs in Current Dev average: 7.275 secs.

[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h: list = "[]"]
[h, count(10000): list = json.append(list, roll.count) ]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
[r: time = t2 - t1]

[Phergus]

Build updated to use Parser 1.8.0 now.

[adventuremagic123]

Is it possible there's also some sort of architectural issue adding to the performance problems.

My players report what appears to be a macro execution time sometimes about 3 times longer or more than what I see when using the same macros from my PC, which is running the MapTool server. The macros that exhibit this behavior tended to have the most code complexity. Most of my macros calls functions from library tokens. I used to store some logging data in a library token for basically everything players did with their tokens, but I found that was hurting performance so I moved all logging to the tokens themselves. Now, I just have party treasure data in a library token. The data issue was an obvious mistake.

When a token calls a library token function, does that turn into a remote call?

As far as I think I understand, tokens are cached locally on the players' PCs. Couldn't library tokens be cached there as well? Generally, mine mostly just contain macros and almost never change during a game session.

[adventuremagic123]

Also, my library tokens as far as I remember are separated into one that contains campaign data and the others that contain very basic properties (such as version) and macros.

[adventuremagic123]

I really think macro performance is the most important issue currently facing MapTool.

[nmeier]

I'll have a look into your scenario next - don't know yet what happens with macros and connected players, where do they run, why is that even slower.

[cwisniew]

Is it possible there's also some sort of architectural issue adding to the performance problems.

My players report what appears to be a macro execution time sometimes about 3 times longer or more than what I see when using the same macros from my PC, which is running the MapTool server. The macros that exhibit this behavior tended to have the most code complexity. Most of my macros calls functions from library tokens. I used to store some logging data in a library token for basically everything players did with their tokens, but I found that was hurting performance so I moved all logging to the tokens themselves. Now, I just have party treasure data in a library token. The data issue was an obvious mistake.

When a token calls a library token function, does that turn into a remote call?

unless specifically coded to run remotely (via macroLink()) macros will run on the client where they are initiated (so on the client that someone pushed the button on for example). Is there a large discrepancy between the players computer and yours?

As far as I think I understand, tokens are cached locally on the players' PCs. Couldn't library tokens be cached there as well? Generally, mine mostly just contain macros and almost never change during a game session.

Library Tokens are no different from other Tokens in this respect.

[Merudo]

I believe tokens on a map are only cached if the player visits the map.

If this is correct, a library token on a non-player accessible map will never be cached, regardless of how often it is called.

[cwisniew]

I believe tokens on a map are only cached if the player visits the map.

If this is correct, a library token on a non-player accessible map will never be cached, regardless of how often it is called.

On Campaign Load (or connection) the contents of the campaign are sent to clients, this will include map details and token details, you can test this by using a trusted function on a non GM client to read tokens off of a hidden map. What is fetched when a player displays a map is all the assets (e.g. images) for the maps and tokens, the token properties should be there already.

[adventuremagic123]

Thanks for the info. Maybe to us it just looked like a long time. I didn't time it. The player reported 3 minutes. I've timed it and seen it take a mintute. Ignore my comment. Probably not valid.

[Phergus]

Build updated to dicelib 1.6.4.
https://github.com/RPTools/dicelib/releases/tag/1.6.4

[nmeier]

Parser/Expression re-creation on every macro line, even in a loop. One could re-use ExpressionParser/Expressions

I have further improvements to address the repeated re-parsing of expressions for scripts. This makes extreme test examples executing loops run in about a second now (which started at 10+seconds at the beginning of this). All expressions are weak cached - think Java's JIT - and run against their own variable resolver.

FYI pull request for parser, dicelib, maptool should be out tomorrow.

[adventuremagic123]

Really great work! Using MapTool is all about the macros. The single biggest complaint I get from players is the slow performance we get in MapTool 1.7.0 and previous versions. On my end I've done almost everything I can to use caching and do things efficiently. This performance boost will give me a much needed break. Thank you!

[Phergus]

Some testing results:

Counting to 10,000

[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h, count(10000): v = roll.count]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
10K Count: [r: (t2 - t1)/1000]

Average of 10 runs:
| MT 1.7 | MT 1.8 |
| ------- | ------ |
| 1.766 secs | 1.545 secs |

Rolling 1d10000-1 10,000 times

[h: t2 =  json.get(getInfo("server"),"timeInMs")]
[h, count(10000), code: {
    [h: i = 1d10000-1]
}]
[h: t3 =  json.get(getInfo("server"),"timeInMs")]
10K Random: [r: (t3 - t2)/1000]<br>

Average of 10 runs:
| MT 1.7 | MT 1.8 |
| ------- | ------ |
| 2.900 secs | 2.591 secs |

Those two tests mostly just show that Java 14 is faster than Java 10.

Appending to an array 10,000 times

[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h: list = "[]"]
[h, count(10000): list = json.append(list, roll.count) ]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
Append: [r: (t2 - t1)/1000]

Average of 10 runs:
| MT 1.7 | MT 1.8 |
| ------- | ------ |
| 8.274 secs | 2.528 secs |

Build an Array with 10,000 elements and then do 10,000 json.get() and json.remove() operations

[h: list = "[]"]
[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h, count(10000): list = json.append(list, roll.count) ]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
Build Array: [r: (t2 - t1)/1000]<br>
[h, count(10000), code: {
    [h: i = 1d10000-1]
    [h: v = json.get(list, i)]
}]
[h: t3 =  json.get(getInfo("server"),"timeInMs")]
JSON Get: [r: (t3 - t2)/1000]<br>
[h, count(10000), code: {
    [h: list = json.remove(list, 0)]
}]
[h: t4 =  json.get(getInfo("server"),"timeInMs")]
JSON Remove: [r: (t4 - t3)/1000]<br>

Average of 10 runs:
| Action | MT 1.7 | MT 1.8 | |
| :------ | ------: | ------: | :------ |
| JSON Get | 17.353 | 5.081 | secs |
| JSON Remove | 8.757 | 2.702 | secs |

Results show that the performance improvements have had a significant effect.

[Phergus]

Parser updated to 1.8.1 to get backwards compatibility with comparison/concatenation operations.
https://github.com/RPTools/parser/pull/42

[nmeier]

Preview of what expression caching can squeeze out - not having the parser re-apply transforms, lexer, construct AST yields the 4th columns (3rd is my baseline). Unfortunately the current change I have on parser will break backwards compatibility. Have spent some time investigating whether the parser API can stay 100% same but that doesn't get a nice looking result. TBD

Counting to 10,000
Average of 10 runs:

| MT 1.7 | MT 1.8 | 1.8 local baseline | 1.8 local expression caching
|-|-|-|-|
1.766 secs |1.545 secs|0.655 secs|0.115 secs

Rolling 1d10000-1 10,000 times
Average of 10 runs:

| MT 1.7 | MT 1.8 | 1.8 local baseline | 1.8 local expression caching
|-|-|-|-|
2.900 secs | 2.591 secs | 1.653 secs | 0.7144 secs

Appending to an array 10,000 times
Average of 10 runs:

| MT 1.7 | MT 1.8 | 1.8 local baseline | 1.8 local expression caching
|-|-|-|-|
8.274 secs| 2.528 secs| 2.1637 secs | 1.2479 secs

Build an Array with 10,000 elements and then do 10,000 json.get() and json.remove() operations
Average of 10 runs:

| MT 1.7 | MT 1.8 | 1.8 local baseline | 1.8 local expression caching
|-|-|-|-|
JSON Get 17.353 | 5.081 secs | 3.0879 secs | 1.2827 secs
JSON Remove 8.757 | 2.702 secs | 1.868 secs | 0.738 secs

[nmeier]

So this is a bigger suggestion to pull in everything triggered by https://github.com/RPTools/parser/issues/44 - requires update in sequence to:

Parser PR https://github.com/RPTools/parser/pull/45

Dicelib PR https://github.com/RPTools/dicelib/pull/57

Maptool PR https://github.com/RPTools/maptool/pull/2086

these changes make caching of parsed expression possible in parser/dicelib. Caching is implemented in MaptoolExpressionParser.

Gives a 2-6 factor improvement for re-running macros (or expressions in a loop) - see https://github.com/RPTools/maptool/issues/1898#issuecomment-656957853

The longer the expression parsed, the more advantageous it is.

[Phergus]

Unfortunately the current change I have on parser will break backwards compatibility.

Please expand on that. Are you saying that these new changes are going to break existing frameworks?

[nmeier]

I could have been clearer: tl;dr Scripts will run the same way, there's no change to frameworks or macros themselves.

What breaks is if any other library/tool was using dicelib or parser. The parser library interface changes externally from

new Parser(resolver, true).parseExpression("Hello").evaluate()
to
new Parser(true).parseExpression("Hello").evaluate(resolver)

The dicelib library changes from
new ExpressionParser(resolver).evalute("Hello")
to
new ExpressionParser().evaluate("Hello", resolver)

The PR for Maptool has the necessary update and uses the distinction between parsing and evaluating to inject caching of already parsed expressions.

All the macros, scripts, and unittests continue to work of course.

[Phergus]

Great! That one line was throwing me and I thought I was missing something in the code changes.

[Phergus]

And I'm guessing that only DiceTool would be affected if anything.

[nmeier]

I can create a fix for dicetool, haven't played with that yet but will check it out

[Phergus]

Not a priority. In theory someone is trying to get it functional again but haven't seen any progress on it.

[cwisniew]

Not a priority. In theory someone is trying to get it functional again but haven't seen any progress on it.

Yes This!
DiceTool has other issues making lates code non usable at the moment so upgrading the parser to latest version is a low priority.

[Phergus]

Tested with latest changes to parser/dicelib and local expression caching.

[Phergus]

@nmeier The parser changes seem to have broken User Defined Functions.

[nmeier]

Will check later tonight. Probably a miss in unittest coverage. Simple test case if possible?

[Phergus]

See campaign macros in attached campaign file.

UDF_Fail_170.cmpgn.zip
(Remove .zip from filename)

The UDF is being added to the UserDefinedFunctions hashmap and can be seen in the getInfo("client") list of UDFs. I'm not seeing where that would have been passed on via the MapToolExpressionParser.

[nmeier]

Thanks, PR for Maptool coming

[Phergus]

Issue with UDFs resolved.

@nmeier New problem. Can no longer set Token properties directly.

1. Default campaign. Place token on map.
2. Impersonate token.
3. Run [r: Strength = 3d6]
4. Strength will be unchanged.

Also doesn't work from a macro.

[Phergus]

PR #2104 fixes issue with setting token properties.

No other issues found.

[Merudo]

Refreshing the character sheet of my CoC framework used to take ~1.5 seconds on my machine, now it only takes ~0.5 seconds.

I'm very pleased with the performance improvements!

[nmeier]

@Merudo I profiled the characters sheet update in your framework and see that it could run faster if you use hidden rolls explicitly. The performance tweaks have some general improvements but for a roll like

[1 + 1]

the expression by default (not h:) is made deterministic and then pretty formatted to be ready to be put into, say, chat

« 1 + 1 = 1 + 1 = 2 »

For what I saw getting done in the sheet it seems that you can mostly add [h: ...] to your macro elements, example: Turn these

[isBomb =  if( getPropertyType(Id)=="Vehicle", 1, 0)]

[ validPCs   = if ( coc.canGetSheet(id)    , listAppend(validPCs  ,id), validPCs   )]
    [ validBombs = if ( coc.canGetExplosive(id), listAppend(validBombs,id), validBombs )]

[return(0,1)]

[coc.openCharacterSheet(1, Id)]


[mySheetCache = encode(coc.buildCharacterSheet())]
    [setProperty("fullSheetCache" ,mySheetCache)]
    [setProperty("UpdateSheet", "No")]

into

[h:isBomb =  if( getPropertyType(Id)=="Vehicle", 1, 0)]

[h: validPCs   = if ( coc.canGetSheet(id)    , listAppend(validPCs  ,id), validPCs   )]
    [ h: validBombs = if ( coc.canGetExplosive(id), listAppend(validBombs,id), validBombs )]

[h: return(0,1)]

[h: coc.openCharacterSheet(1, Id)]


[h: mySheetCache = encode(coc.buildCharacterSheet())]
    [h: setProperty("fullSheetCache" ,mySheetCache)]
    [h: setProperty("UpdateSheet", "No")]

Would be interested to see the result for you. I hacked in a 'always hidden' roll and the sheet update goes from 1s to ~100ms on my laptop.

[Merudo]

That's awesome, @nmeier!

I was under the impression that calling a macro with output set to none was equivalent to adding h: everywhere. I see now that this is not the case, and that adding h: can result in substantial speed gains.

Should I add h: to statement within loops too? For example, if I have the code

[h, while(condition),CODE:
{
 [statement1]
 [statement2]
}]

Can it be made faster by adding h: to the inner statements?

[h, while(condition),CODE:
{
 [h: statement1]
 [h: statement2]
}]

[nmeier]

@Merudo as far as I can see any macro statement in bracket is parsed for the roll option, and no h: means there's extra cycles for building a deterministic expression, and formatting - even in your while loop where the result is not needed. I'm investigating if your macro with output to none

macro://ClickImage@Lib:coc/none/A55473365EA24BF0A3E5D8389182D35F?
could be respected automatically - if noone uses the result in the end, skip all output formatting and gathering.

[Phergus]

@nmeier Was wondering about the impact of hiding the output.

No Code block
[h, count(10000): v = roll.count]
With h, avg of 10 runs: 0.0419 secs

[count(10000): v = roll.count]
W/o h, avg of 10 runs: 0.0903 secs

Twice as long with the h but the time is insignificant either way even with 10k iterations.

However this is more interesting
Using a Code Block with Assignment in Square Brackets

[h, count(10000), code: {
    [h: v = roll.count]
}]

Avg of 10 runs: 0.7423 secs
Putting the single assignment in a code block takes nearly 20x as long. Though, again, we are talking less than a second for 10k iterations. Leaving out the h only adds a couple hundredths of a second the average time.

It gets more interesting.

Each simple assignment operation added inside the code block adds about 0.65 secs to the runtime.

Count 10k, 1 assignment, Avg of 10 runs:   0.7472 secs
Count 10k, 2 assignments, Avg of 10 runs:  1.3794 secs
Count 10k, 3 assignments, Avg of 10 runs:  2.0791 secs
Count 10k, 4 assignments, Avg of 10 runs:  2.7251 secs
Count 10k, 5 assignments, Avg of 10 runs:  3.3389 secs
Count 10k, 10 assignments, Avg of 10 runs: 6.6332 secs 

For comparison purposes, same run under MT 1.7. Ouch.

Count 10k, 1 assignment,  Avg of 10 runs:   2.5726 secs
Count 10k, 2 assignments, Avg of 10 runs:   4.6439 secs
Count 10k, 3 assignments, Avg of 10 runs:   6.8734 secs
Count 10k, 4 assignments, Avg of 10 runs:   9.1396 secs
Count 10k, 5 assignments, Avg of 10 runs:  11.4152 secs
Count 10k, 10 assignments, Avg of 10 runs: 22.1639 secs

If you need to do multiple, independent operations in a loop you're probably better off with multiple loops and no code blocks. These are extreme cases so the impact on the average framework macro will likely be very small.

[Merudo]

I hacked in a 'always hidden' roll and the sheet update goes from 1s to ~100ms on my laptop.

I tried but couldn't reproduce your good results.

I added a h where you indicated, plus in a few other key places. The sheet update went from 0.5 seconds to 0.45 seconds, a nice improvement but not one nearly as dramatic as yours.

[nmeier]

@Merudo i'll have to have another look at where exactly you're hit in the framework

[nmeier]

@Phergus yeah, there is more room for improvement. I'm working on a change that addresses repeated parsing of the roll options (every h: or r: is parsed over and over again). This is a tax on every single assignment in a roll block that you've noticed.

This saves in regex compilation (also made a couple of Pattern.compile static where possible)

My results right now

[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h, count(10000), code: {
    [h: v = roll.count]
}]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
[r: time = t2 - t1] ms

| | current development branch | my changes|
|--|--|--|
| avg of 10 runs | 516 ms | 115 ms |

[h: t1 = json.get(getInfo("server"),"timeInMs")]
[h, count(10000), code: {
    [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] [h: v = roll.count] 
}]
[h: t2 =  json.get(getInfo("server"),"timeInMs")]
[r: time = t2 - t1] ms

| | current development branch | my changes|
|--|--|--|
| avg of 10 runs | 5398 ms | 534 ms |

There are more regex compilation that I wish weren't there but that's a pretty complex code path for how branchRegex, branchSepRegex, branchRegex, branchLastSepRegex are assembled at runtime.

I'll create a new issue for this specifically.

[Phergus]

That's great.