Skip to content

Commit

Permalink
nativelib regex RE2.Machine mutual exclusion improvements, re2j PR 85…
Browse files Browse the repository at this point in the history
… & 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.
  • Loading branch information
Lee Tibbert authored and jchyb committed Oct 4, 2021
1 parent c7a645a commit 84abe40
Showing 1 changed file with 33 additions and 9 deletions.
42 changes: 33 additions & 9 deletions nativelib/src/main/scala/scala/scalanative/regex/RE2.scala
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ class RE2 private {

// Cache of machines for running regexp.
// Accesses must be serialized using |this| monitor.
// @GuardedBy("this")

/// ScalaNative Porting Note:
/// re2j PR #85 changed the data type of machine field to ArrayDeque.
/// That type is not yet implemented on ScalaNative, so retain declaration
/// prior to that PR; it has worked well for months.

private val machine = new ArrayList[Machine]()

// This is visible for testing.
Expand Down Expand Up @@ -98,25 +105,42 @@ class RE2 private {

// get() returns a machine to use for matching |this|. It uses |this|'s
// machine cache if possible, to avoid unnecessary allocation.
def get(): Machine = synchronized {
val n = machine.size()
if (n > 0) {
return machine.remove(n - 1)

def get(): Machine = {
/// See ScalaNative Porting note where machine field is declared above
/// for the reason why the ArrayList datatype from before re2j PR #85
/// is retained.

/// Having two return statements is an eyesore but it _vastly_ simplifies
/// the mutual exclusion logic.

this.synchronized {
val n = machine.size()
if (n > 0) {
return machine.remove(n - 1)
}
}
return new Machine(this)

return new Machine(this);
}

// Clears the memory associated with this machine.
def reset(): Unit = synchronized {
machine.clear()
def reset(): Unit = {
// Interior synchronized block to work around SN Issue #1091
this.synchronized {
machine.clear()
}
}

// put() returns a machine to |this|'s machine cache. There is no attempt to
// limit the size of the cache, so it will grow to the maximum number of
// simultaneous matches run using |this|. (The cache empties when |this|
// gets garbage collected.)
def put(m: Machine): Unit = synchronized {
machine.add(m)
def put(m: Machine): Unit = {
// Interior synchronized block to work around SN Issue #1091
this.synchronized {
machine.add(m)
}
}

override def toString: String = expr
Expand Down

0 comments on commit 84abe40

Please sign in to comment.