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

Adds more multithreaded access testing to StructTest. #767

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

tgregg
Copy link
Contributor

@tgregg tgregg commented Mar 12, 2024

Description of changes:

Adds multithreaded access testing to the relevant hand-written tests in StructTest and the generated test testRandomChanges().

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 1629 to 1630
// Concurrently access all fields in the struct using many threads, recording the results in a concurrent
// multi-map.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the guards in the ConcurrentHashMap and the synchronized list have any effects on the execution of the CompletableFutures?

If so, you might want to consider something like this, and have task i store its results directly into results[i].

Map<String, IonValue>[] results = new HashMap<String, IonValue>[numberOfTasks];

@tgregg tgregg force-pushed the multithreaded-structtest branch from 787b1e4 to 4d7613c Compare April 19, 2024 15:58
@tgregg
Copy link
Contributor Author

tgregg commented Apr 19, 2024

Revision 2:

  • Address Matt's comment by removing synchronization of results collection.
  • Added a direct test for a scenario that happened infrequently in testRandomChanges. Also increased the number of iterations of testMultipleRandomChanges until this scenario was produced consistently. This scenario revealed an incorrect assertion in revision 1, which relied on IonStruct.get to return the same value for a given name in all circumstances, even when there were multiple mappings for that name. However, the documentation for IonStruct.get says "If the field name appears more than once, one of the fields will be selected arbitrarily". Therefore, I needed to add the assertStructContainsMapping function to allow for any value for the requested name to be considered acceptable.

@tgregg tgregg merged commit 2d72415 into master Apr 19, 2024
23 of 41 checks passed
@tgregg tgregg deleted the multithreaded-structtest branch April 19, 2024 16:36
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.

2 participants