Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix setTableEntry() performance #4770

Draft
wants to merge 19 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Azhrei
Copy link
Member

@Azhrei Azhrei commented Apr 30, 2024

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 LookupTable implementation was modified to add a setTableEntry() method that will overwrite an existing entry. The original implementation used addTableEntry() 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

  • fixed performance issue with setTableEntry() when used with large tables

This change is Reviewable

Azhrei and others added 5 commits February 16, 2024 22:07
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.
@FullBleed
Copy link

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.

Azhrei and others added 14 commits May 8, 2024 23:49
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?
@Azhrei Azhrei marked this pull request as ready for review May 26, 2024 22:49
}
// public String getRoll() {
// return getDefaultRoll();
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove function rather than just commenting it out

*
* @param name name of the table
*/
public void setName(@org.jetbrains.annotations.Nullable String name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are IntelliJ recommendations. It apparently thinks they are not equivalent and wouldn't allow one to be used in place of the other without producing a warning. I wanted to eliminate all warnings, so... Maybe it's a bug in the version of IntelliJ? I can update and try again.

} 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'm surprised IntelliJ didn't suggest that.

lookupTable.setTableImage(asset);
MapTool.serverCommand().updateCampaign(MapTool.getCampaign().getCampaignProperties());
return "";
private @Nullable Serializable tbl_and_table(String function, List<Object> params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the function does not follow naming standards


} else if ("setTableEntry".equalsIgnoreCase(function)) {
assert result.getValue() != null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not follow formatting standards. The same formatting standards issue crops up multiple places in this change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, shouldn't spotlessApply take care of the formatting? Running spotless was the last commit in the PR.

(Or maybe there are things spotless doesn't correct/change and must be done manually? If so, is there a list of such things that spotless doesn't fix but it should? Apparently single-line IF statements is one of them; what about using braces when a true or false block only contains a single statement?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Google formatting library that Spotless uses doesn't deal with single statement ifs because of reasons... (to keep Android developers happy as the coding guidelines for Android are different on that one point).

There may be others but they haven't come up before (this one has come up several times)

return counter == rowindex ? new JsonPrimitive(counter) : errorRows;
}
// FIXME Report error using I18N
throw new ParserException("Second parameter must be a JSON Array");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be extracted to i18n

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azhrei I think this may work (so as to maybe get this in to 1.15.1.
throw new ParserException( I18N.getText("macro.function.json.onlyArray"));

@Azhrei Azhrei self-assigned this Oct 11, 2024
@Azhrei
Copy link
Member Author

Azhrei commented Oct 11, 2024

Hurricane Milton knocked out power, but I'll try to get to this before the end of the weekend, assuming power is back up. Otherwise, I'll convert it to Draft, which I think takes it out of this queue...

@Azhrei Azhrei marked this pull request as draft October 14, 2024 01:54
@Azhrei
Copy link
Member Author

Azhrei commented Oct 14, 2024

It looks like changing the PR to Draft only disables the ability to perform a Review, but doesn't make it disappear out of the PR queue. (I had hoped that it would only appear to the person(s) assigned to it.) I likely won't be able to work on it until 10/15 ET, so close it if you feel it's necessary and I'll resubmit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: setTableEntry extremely slow
4 participants