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

Prevent messages persisting between iterations #973

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Nov 11, 2022

Message lists previously would persist from one iteration to the next, due to comparable behaviour in FLAME GPU 1.

This semi-breaks the intended state machine, but is potentially a useful optimiation in some very edge cases.

This PR (now) implements message lists persistence, through a property on the messageDescription object, e.g.

MessageBruteForce::Description message = model.newMessage("msg");

// Query the persistence, false by default
bool isPersistent = message.getPersistent();

// Set the message list to persist iterations
message.setPersistent(true);

Message lists which are not marked as persistent are set to 0 length and the appropraite flags set to rebuild data structures prior to their next use at the end of each step (i.e. lazy resetting to avoid unneccesary memsets).

  • Implement getPersistent
  • Implement setPersistnt
  • Test getting/setting persistnece in C++
  • Test getting/setting persistence in python
  • Test persistence in C++
    • Just Bucket so far, others can be deferred to future tests?
  • Test persistence in Python
  • Docs issue opened Message list persistence FLAMEGPU2-docs#125

Closes #247


Original PR text for posterity

  • Add test(s) which detect messages persisting from one iteration to the next
  • Prevent messages persisting from one iteration to the next (via resetting at the end of the iteration)

This is based on 972 due to common test code.

@ptheywood ptheywood force-pushed the non-persistent-messagelists-only branch from 082f430 to a1b25e4 Compare November 11, 2022 16:40
@ptheywood ptheywood marked this pull request as ready for review November 11, 2022 16:40
@ptheywood ptheywood requested review from Robadob and mondus November 11, 2022 16:40
@ptheywood
Copy link
Member Author

Full test suite passes for release with belts under linux:

[----------] Global test environment tear-down
[==========] 1032 tests from 80 test suites ran. (165641 ms total)
[  PASSED  ] 1032 tests.

  YOU HAVE 40 DISABLED TESTS

If any models rely on persistence then this will break them however.

@Robadob
Copy link
Member

Robadob commented Nov 11, 2022

If any models rely on persistence then this will break them however.

Yeah I don't agree with this change. Add a HostAPI method for users to manually purge message lists, and bin this PR.

@ptheywood ptheywood force-pushed the non-persistent-messagelists-only branch from a1b25e4 to 39b33d9 Compare November 16, 2022 18:37
@ptheywood ptheywood force-pushed the non-persistent-messagelists-only branch 2 times, most recently from 7f7d974 to 7a036c0 Compare November 17, 2022 16:11
@ptheywood
Copy link
Member Author

This is now ready for review, as a PR which makes non-persistent messages default, with opt-in to persistence for each message list.

Adds C++ and python tests for new features, but only checks persistence behaves correctly for brute force messages and bucket messages (in the interest of time, they are not simple tests).

Message lists are now non-persistent by default (messages from the previous iteration are no longer available prior to output in the current iteration)

message lists can be marked as persistent by calling .getPersistent() on the description object.

Adds brute-force tests to check get/set works as intended

Adds brute-force tests for persistence / non-persistence working on a multi-iteation simulation

Adds Bucket persistence / non-persistence tests (C++ only)
This fixes a swig linker issue on Windows CI, which is somehow related to the using statements.
@ptheywood ptheywood force-pushed the non-persistent-messagelists-only branch from 3f49f2c to 5f4f9c7 Compare November 21, 2022 16:14
Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

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

Haven't tested personally, but looks thorough (you have a Python test which would have been my main concern).

@mondus mondus merged commit de88c97 into master Nov 23, 2022
@mondus mondus deleted the non-persistent-messagelists-only branch November 23, 2022 15:15
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.

Persistent / Non-Persistent Message Lists
3 participants