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.

Sign up for GitHub

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

Jump to bottom

Hashing tests #5026

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Hashing tests #5026

wants to merge 3 commits into from

Conversation

ebbit1q
Copy link
Member

@ebbit1q ebbit1q commented Apr 29, 2024

Related Ticket(s)

Short roundup of the initial problem

There are no tests that verify the deck hasher remains the same.

What will change with this Pull Request?

  • adds tests
  • tries to improve performance

@ebbit1q
Copy link
Member Author

ebbit1q commented Apr 29, 2024

the tests are adequate but I am not satisfied with my attempt at improving performance

before:

[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from DeckHashTest
[ RUN      ] DeckHashTest.RepeatTest
[       OK ] DeckHashTest.RepeatTest (8168 ms)
[ RUN      ] DeckHashTest.NumberTest
[       OK ] DeckHashTest.NumberTest (616 ms)
[ RUN      ] DeckHashTest.UniquesTest
[       OK ] DeckHashTest.UniquesTest (2831 ms)
[----------] 3 tests from DeckHashTest (11617 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (11617 ms total)
[  PASSED  ] 3 tests.

after:

[==========] Running 3 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 3 tests from DeckHashTest
[ RUN      ] DeckHashTest.RepeatTest
[       OK ] DeckHashTest.RepeatTest (4033 ms)
[ RUN      ] DeckHashTest.NumberTest
[       OK ] DeckHashTest.NumberTest (93 ms)
[ RUN      ] DeckHashTest.UniquesTest
[       OK ] DeckHashTest.UniquesTest (6718 ms)
[----------] 3 tests from DeckHashTest (10845 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test suite ran. (10845 ms total)
[  PASSED  ] 3 tests.

we see the total improvement over all 3 tests is negligible because while the first two are faster the last is much slower.

@ZeldaZach
Copy link
Member

I still think we need to limit the size of the input field, as there's no reasonable explanation for having more than, say, 100,000 cards in a deck.

@ebbit1q
Copy link
Member Author

ebbit1q commented Apr 29, 2024

I agree, I'm thinking of whether to make it a per node limit (easy to implement), or a total card limit, but I think it should be a limit on the deck itself, not just the hash.

ZeldaZach
ZeldaZach reviewed May 4, 2024
View reviewed changes
common/decklist.cpp
// all cards are sorted, yet as all cards are made lower case and SB: is added to all sideboard cards, they will
// always sort before mainboard cards

for (auto rootIter = root->cend() - 1, rootEnd = root->cbegin() - 1; rootIter != rootEnd; --rootIter) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the benefit of this over for (auto rootValue : root) ?

ZeldaZach
ZeldaZach reviewed May 4, 2024
View reviewed changes
common/decklist.cpp
for (auto i = sbCardList.cbegin(), end = sbCardList.cend(); i != end; ++i) {
for (int j = 0; j < i.value(); ++j) {
if (started) {
hasher.addData(";SB:");
Copy link
Member

Choose a reason for hiding this comment

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

Does CPP not have a ";".join(sbCardList) equiv?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeldaZach ZeldaZach ZeldaZach left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ebbit1q @ZeldaZach