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 InvalidMutabilityException in RegexToken #41

Merged
merged 3 commits into from Aug 3, 2021

Conversation

itegulov
Copy link
Contributor

Mutating relativeInput makes better-parse throw InvalidMutabilityException on iosX64 (probably on other Kotlin/Native platforms as well). This PR turns relativeInput into a local immutable object.

@@ -40,8 +37,7 @@ public actual class RegexToken : Token {
}

override fun match(input: CharSequence, fromIndex: Int): Int {
relativeInput.input = input
relativeInput.fromIndex = fromIndex
val relativeInput = RelativeInput(fromIndex, input)
Copy link
Owner

Choose a reason for hiding this comment

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

After this change, an instance of RelativeInput is created on each invocation of match, which, if not properly optimized, might cause a performance drop. I'd like to check how the benchmarks run on a native platform before and after the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, benchmarks subproject does not seem to support native targets. Do you want me to add one?

@h0tk3y
Copy link
Owner

h0tk3y commented May 4, 2021

Also, how does the InvalidMutabilityException appear? Is it some native-multithreading scenario? Could you please add a test for the native platforms that reproduces the issue or describe the case so that I can add the test myself?

@itegulov
Copy link
Contributor Author

@h0tk3y sorry, the last month was very busy for me. I have added a test showing how to reproduce the error in concurrent scenario.

@itegulov
Copy link
Contributor Author

itegulov commented Jul 1, 2021

@h0tk3y ping

@h0tk3y
Copy link
Owner

h0tk3y commented Aug 3, 2021

It seems like the fix doesn't affect the Kotlin/Native benchmarks in any noticeable way. I'm accepting it now and will publish a new release sometime soon. Thanks a lot!

@h0tk3y h0tk3y merged commit 29ed5f2 into h0tk3y:master Aug 3, 2021
@itegulov
Copy link
Contributor Author

@h0tk3y thanks for merging the PR. How soon can we expect the new version to be released?

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.

None yet

2 participants