fix setTableEntry() performance
#4770
Conversation
Disabled nightly build by changing an element that would normally schedule a job.
FullBleed complained on Discord about the performance of this MTscript function. It turns out that the algorithm is O(n), so putting 3200 entries in a table gets seriously slow. Fixing this will require the supporting `LookupTable` class to have its own `setTableEntry` method. This commit is some simpler formatting and refactoring fixes; the next commit will be more substantial in regards to the actual fix.
|
Nice. I'm going to wait for this to get in before trying to build my nearly 7000 record NPC table (virtually every stat blocked NPC in every published PF1e book through 2020). As is, I'm not even sure I could do it with my 64GB system without significant pain and suffering. Much obliged. |
Part 1, laying the groundwork by adding documentation and correcting warnings in the affected file(s). WIP.
Finished adding tests of the model. I'm not thrilled with the ExpressionParser class being used directly by LookupTable, but changing it to use DI is beyond the scope of this issue. When the LookupTable changes its implementation to a sorted array, the dependency can be changed (perhaps). Should the MTscript functions have tests as well? Or should those be done via a macro within the tool? It would certainly be easier to automate if they were in Java, but putting a bunch of testing macros into a campaign would (potentially) allow others to build tests as well and contribute to the project. (TODO) With the use of `macro.catch`, maybe exceptions thrown within MTscript can actually be caught and handled in a sufficient way?
This reverts commit ee9a535.
This reverts commit e1796d2.
| } | ||
| // public String getRoll() { | ||
| // return getDefaultRoll(); | ||
| // } |
There was a problem hiding this comment.
Please remove function rather than just commenting it out
| * | ||
| * @param name name of the table | ||
| */ | ||
| public void setName(@org.jetbrains.annotations.Nullable String name) { |
There was a problem hiding this comment.
There is a mixture of @nullable and @org.jetbrains.annotations.Nullable, even the imported one is the same class as the fully qualified one (which I can't tell if it is from the review) it's confusing to anyone reading the code. This is occurring in multiple places in the file
| } catch (ParserException e) { | ||
| MapTool.showError("Error getting default roll for Pick Once table ", e); | ||
| return (NO_PICKS_LEFT); | ||
| MapTool.showError("Error getting default roll for Pick Once table: " + name, e); |
There was a problem hiding this comment.
I know it wasn't before but since you are changing the message this is perfect opportunity to put it into the i18n properties file
| } else { // if tbl, table, tblImage or tableImage | ||
| return tbl_and_table(function, params); | ||
| } | ||
| } |
There was a problem hiding this comment.
It's clearer if you convert the function variable to lower (or upper) case and then just use a switch statement, or better yet a switch expression if you are just going to return the value of a function in each else if.
| lookupTable.setTableImage(asset); | ||
| MapTool.serverCommand().updateCampaign(MapTool.getCampaign().getCampaignProperties()); | ||
| return ""; | ||
| private @Nullable Serializable tbl_and_table(String function, List<Object> params) |
There was a problem hiding this comment.
The name of the function does not follow naming standards
|
|
||
| } else if ("setTableEntry".equalsIgnoreCase(function)) { | ||
| assert result.getValue() != null; |
There was a problem hiding this comment.
If this should never happen, please place a comment describing why so people reading the code know its not just a mistake that should be replaced, as otherwise, it should be a more meaningful error reported.
| LookupTable lookupTable = checkTableAccess(name, function); | ||
| String roll = params.get(1).toString(); | ||
| LookupEntry entry = lookupTable.getLookupDirect(roll); | ||
| if (entry == null) return ""; |
There was a problem hiding this comment.
Does not follow formatting standards. The same formatting standards issue crops up multiple places in this change
| return counter == rowindex ? new JsonPrimitive(counter) : errorRows; | ||
| } | ||
| // FIXME Report error using I18N | ||
| throw new ParserException("Second parameter must be a JSON Array"); |
Identify the Bug or Feature request
Closes #4769
Description of the Change
The original code for
setTableEntry()was executing an O(n) algorithm to add a single element to the table, causing massive slowdown on larger tables as well as excessive memory churn (OP claimed 32-48GB was required to add 3200 elements to a table).To correct this, the original
LookupTableimplementation was modified to add asetTableEntry()method that will overwrite an existing entry. The original implementation usedaddTableEntry()to copy the existing table to a new one as overwriting a single entry was not possible.Also included are refactoring of redundant code blocks into a private method,
checkTableAccess(), as well as refactoring to move large code blocks for each subfunction into their own private methods.Possible Drawbacks
There shouldn't be any.
Documentation Notes
None.
Release Notes
setTableEntry()when used with large tablesThis change is