Greedy String Tiling out of bounds #2179
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… racing conditions
|
Kr0nox
reviewed
Feb 6, 2025
| * @param options determines the parameterization. | ||
| * @deprecated in favor of static {@link #run(JPlagOptions)}. |
There was a problem hiding this comment.
did this move down due to spotless?
There was a problem hiding this comment.
Yeah. I'm pretty sure I didn't touch the Javadoc. But I think this is the canonical order.
Kr0nox
reviewed
Feb 6, 2025
| * @return the results of the comparison, specifically the submissions whose similarity exceeds a set threshold. | ||
| * @throws ExitException if JPlag exits preemptively. | ||
| * @deprecated in favor of static {@link #run(JPlagOptions)}. |
Kr0nox
reviewed
Feb 6, 2025
| return this.tokenValueMap.get(submission); | ||
| } | ||
|
|
||
| public static TokenValueMapper generateTokenValueMapper(SubmissionSet submissionSet) { |
There was a problem hiding this comment.
Is there a special reason to have this generator method and not use the constrcutor?
There was a problem hiding this comment.
Kind of. Usually constructors should not contain application logic, at least the way I learned it. So having a constructor create the entire map automatically feels like it's bad code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moved the token value mapping outside greedy string tiling to prevent racing conditions
The out of bounds exception that occasionally occurs is presumably caused by calculation the token values in multiple threads and using non-thread-safe methods. This PR solves that problem by calculating the lists before starting greedy string tiling.
I took some measurements and the total time for JPlag seems to be unaffected
On the progpedia data I got The following measurement:
old: 2.053 s ± 0.014 s
new: 2.080 s ± 0.016 s
Using 100 copies of JPlag core code as input:
old: 13.006 s ± 0.201 s
new: 12.997 s ± 0.150 s