nativelib regex RE2.Machine mutual exclusion improvements, re2j PR 85 & more #1692
Conversation
This file contains hidden or 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
added 2 commits
May 22, 2020 11:42
… & more * This PR ports [re2j](https://github.com/google/re2j/) PR #[85](google/re2j#85) "Don't create Machine while holding this' monitor", dated 2019-02-22, to Scala Native. My thanks and appreciation to the re2j developer @sjamesr for the re2j PR. * Since ScalaNative is currently single threaded, this PR may seem to be not worth the overhead. At the very least, it keeps the scalanative.regex/re2s implementation close to the re2j parent. In the longer run, I do believe that someday, Real Soon Now, a Big Switch will be thrown and not having synchronization issues that could & should have been avoided sprinkled all over the code base will be seen to be a Good Thing. * The port looks slightly different than one would expect from the normal java-to-scala differences. The essence of the re2j PR is maintained but there is a difference in implementation. ScalaNative does not yet implement the ArrayDeque introduced by re2j in its PR. This port retains the ArrayList implementation used by re2j before the PR and by re2s/sn_regex. The ArrayList has worked well for sn_regex. I studied ArrayDeque and it seemed to offer no obvious performance advantages, given that RE2.get() is implemented to remove the last element of the list. * ScalaNative Issue scala-native#1091 describes a bug where monitor methods are not generated when `synchronized` is used with `def`. It reports that the code generated for interior synchronized blocks is correct. There are three uses of `synchronized` in the scalanative.regex package. + This PR naturally changed RE2.get(). + RE2.reset() and RE2.put() were changed and the reason commented. * No unit-tests were added. The re2j PR had no changes associated with testing. Focus testing the behavior on a single threaded ScalaNative would be hard. While it can not be shown that this PR introduced the positive change it describes, it can be shown, to a reasonable degree of certitude, that it did not introduce obvious negative changes, a.k.a. bugs: `test-all` in release-fast mode succeeds with no new performance issues. Documentation: * The standard changelog entry is requested. Testing: * Built and tested ("test-all") in release-fast mode using sbt 1.2.8 on X86_64 only . All tests pass.
Use ArrayDeque from recent SN PR scala-native#1696 to match the changes in the re2j PR. Eliminating SN divergence should make it easier to merge some known re2j changes planned for two or more rounds out.
e439d20 to
67f8608
Compare
Rebased 2020-05-22
67f8608 to
962e6ff
Compare
This was referenced Oct 4, 2021
|
Rebased and merged in #2402. Thank you for publishing the original solution |
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.
This PR ports re2j PR #85 "Don't create Machine while holding this' monitor",
dated 2019-02-22, to Scala Native.
My thanks and appreciation to the re2j developer @sjamesr for the
re2j PR. The porting is my original contribution.
Since ScalaNative is currently single threaded, this PR may seem
to be not worth the overhead. At the very least, it keeps the
scalanative.regex/re2s implementation close to the re2j parent.
In the longer run, I do believe that someday, Real Soon Now, a
Big Switch will be thrown and not having synchronization issues
that could & should have been avoided sprinkled all over the
code base will be seen to be a Good Thing.
ScalaNative Issue Monitor.enter and Monitor.exit not generated for synchronized methods #1091 describes a bug where monitor methods
are not generated when
synchronizedis used withdef.It reports that the code generated for interior synchronized blocks
is correct.
There are three uses of
synchronizedin the scalanative.regex package.This PR naturally changed RE2.get().
RE2.reset() and RE2.put() were changed and the reason commented.
I do not understand why the upstream re2j changed the data structure from
ArrayList to ArrayDeque, then uses it as a Queue. Seems like a slight
pessimization, if ArrayDeque is implemented in terms of ArrayList, as
it now is in Scala Native.
It seemed kinder to future devos, including myself in three months, to follow
the re2j change that to maintain lots of comments explaining why re2s/sn.regex
and re2j differed. This is especially important because of rounds of re2j PRs I hope
to port in the near future.
No unit-tests were added. The re2j PR had no changes associated
with testing. Focus testing the behavior on a single threaded
ScalaNative would be hard.
While it can not be shown that this PR introduced the positive
change it describes, it can be shown, to a reasonable degree of
certitude, that it did not introduce obvious negative changes,
a.k.a. bugs:
test-allin release-fast mode succeeds with no newperformance issues.
Documentation:
Testing:
X86_64 only . All tests pass.