Re-implement PrintStream.print(obj: AnyRef) to avoid a CharSequence -> String conversion

[david-bouyssie]

This PR is WIP to fix #2906
This issue is focusing on the PrintStream use case, but there are maybe other places in the code where using CharSequence instead of String could be beneficial.

This PR reimplements the method PrintStream.print(obj: AnyRef) in order to call encoder.append(csq) instead of encoder.write(s), encoder being an OutputStreamWriter.
This allows to avoid the current conversion of CharSequence to String, and should thus save both CPU and memory consumption.

This PR is a nice workaround to PR #2908, which is proposing a reimplementation of AbstractStringBuilder.toString, AbstractStringBuilder being one possible CharSequence of javalib. But it only works for StringBuilder/StringBuffer used in this context. The PR #2908 tries to solve the allocation issue in more generic way (through a carefully tuned sharing strategy).

The present PR has however the advantage of fewer consequences in terms of logic (no side effect).

[LeeTibbert]

First, thank you for the contribution.

I am at the end of a long sprint right now so I can not do the obvious and try it out.
Given a String("xyz") & :

  def print(s: String)String): Unit = printString(if (s == null) "null" else s)
  def print(obj: AnyRef): Unit = {
    obj match {
      case csq: CharSequence => printCharSequence(csq)
      case _                 => printString(String.valueOf(obj))
    }

I suspect that the print(s:String) will get called, because it is more specific.

Since a String is a kind of CharSequence, what happens if the print(s: string)
is commented out and a case is added as the first case in the match
to case s: String => printCharSequence(if (s == null) "null" else s)

Am I being silly or missing something? Does it have any utility or benefit?
Sorry to take your time if I am dead wrong.

The rest of the code looks good to me. It is still simmering in CI as I
sign-off, so I can not tell if it survived.

[david-bouyssie]

Sharp eyes.
Actually, before committing this, I was wondering why def print(s: String)String): Unit and def print(obj: AnyRef): Unit do not have exactly the same specifications:

I have the feeling it's a bit inconsistent.
However, I double checked in Harmony and Luni and they follow the same rules:
https://android.googlesource.com/platform/libcore2/+/master/luni/src/main/java/java/io/PrintStream.java#412

Disturbing, isn't it?
The Java Lord works in mysterious ways 😆

Now to reply precisely to your question:

• commenting the line def print(s: String)String): Unit, will force to enter def print(obj: AnyRef): Unit
• a null: CharSequence will be treated by the case _ path, even it's also not intuitive to me. I guess it's because Scala will consider that a null has in reality a Null type?
• the executed printString(String.valueOf(obj)) will finally lead to a result similar to def print(s: String)String): Unit
  It's because of this:
  https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/lang/String.scala#L1584

Tested in Scastie with:

class Printer {
  //def printThis(s: String): Unit = println(if (s == null) "null" else s)
  def printThis(obj: AnyRef): Unit = {
    obj match {
      case csq: java.lang.CharSequence => println("csq")
      case _                 => { println("bar") ; println(String.valueOf(obj)) }
    }
  }

}

var csq: CharSequence = new StringBuilder("test")
csq = null
new Printer().printThis(csq)

[LeeTibbert]

The Java Lord works in mysterious ways
Amen!

So the print(string) constructor is necessary.

I think I am missing something about the main idea of this PR: increase performance by reducing allocations.
Sorry to be dense.

By my tracing of case csq: java.lang.CharSequence of this PR & OutputStreamWriter and Writer
from the current Repository:

encoder.append() -> OutputStreamWriter.append. OSW inherits it append from Writer, which has
the toString call.

 def append(csq: CharSequence): Writer = {
    write(if (csq == null) "null" else csq.toString)
    this

Sorry, I am missing where a "CharSequence -> String" conversion is being avoided in the
proposed code. Is there some Work In Progress piece that completes the picture?

I am supportive of the idea & goal.

---

For those following along:
With the AbstractStringBuffer changes proposed in PR #2908, all of the CSQ toString()
should become not free but relatively cheap. Approximately 16 bytes for the String overhead,
with expected, not guaranteed, amortized cost of 0 for its interior Array[Char].

Still, avoiding even a 16 byte allocation is worth it if it can be done within economic bounds.

The current path for a CharSequence, say StringBuilder, goes through String.valueOf(AnyRef)

def valueOf(value: AnyRef): _String =
    if (value != null) value.toString else "null"

Both do a toString().

The append() of the current PR ends up a the same underlying
code as the print(string) call.

def write(str: String): Unit =
    write(str.toCharArray)

[LeeTibbert]

If we could come up with an Array[Char] from the CharSeq and use
the write method which takes that argument (pretty low on the call chain),
we might be making progress.

Java 8 CharSequence has no public toArray, so that probably means
allocating an Array[Char] and filling it in using the CharSequence#chars method.
The total allocation is probably 16 bytes, and one GC allocate call less that a toString() call
would have caused. Could add up, if called enough.

I suspect that over time, as CharSequence work is done, there will be a general need for
such a private method.

I am off to remind myself how String fills in its Array[Char] and how write uses it (I think).
Later: Writer calls: str.toCharArray(). String` has that method.

def toCharArray(): Array[Char] = {
    val buffer = new Array[Char](count)
    System.arraycopy(value, offset, buffer, 0, count)
    buffer
  }

Later:
CharSequence interface is used in only a few places
CharBuffer, Segment, String, StringBuffer, StringBuilder

I find no Segment.scala in the code base.
java.nio.CharBuffer is implemented and has a section of private methods: would need implementation.

String implements toCharArray() already.

StringB* could implement an private abstract method inherited from CharSequence,
They would probably need to do an Array allocation and copy.

I think this, plus the current PR, would save one String (and interior Array[Char]) allocation & copy of chars
and a possibly a few calls in Writer where one kind of write() call a lower layer. Would have to count those.
An Array[Char] allocation and subsequent copy into it would still be done, but that is being done today in
the String.toCharArray call.

[david-bouyssie]

Sorry, I am missing where a "CharSequence -> String" conversion is being avoided in the
proposed code. Is there some Work In Progress piece that completes the picture?

It looks like I was over confident in the ability of Writer.append(csq: CharSequence) to handle things without further conversion.
I should have followed the rabbit until the dead end of the hole...
I'm totally guilty, and I'm very grateful for your serious review, as usual.

I'll commit quickly an updated version.

[LeeTibbert]

| I'll commit quickly an updated version.

I think this is not a sprint and not a marathon but rather than an ultra-marathon.

My suggestion is to take your time & enjoy the puzzle.

Thank you for confirming my tracing. I always wonder if I am seeing what I think I am seeing.

[david-bouyssie]

I hope this will make it.

OutputStreamWriter.writeCharBuffer should do the trick 🤞
Here is the magic line:
val result = enc.encode(cbuf1, outBuf, false)
I have not followed the rabbit inside this call... but might be worth it.

I don't like some lines there though:
val fullInput = CharBuffer.wrap(inBuf + cbuf.toString)
inBuf = cbuf1.toString

[LeeTibbert]

David,

I have not lost interest. It is going to take me a few days to review your latest proposal.
I have to get a few things done here. Sorry for introducing a bubble in the pipeline.

Some thoughts from over the weekend, against the background
of my not having read your latest work in detail. I may be way behind
or just way off. Not to distract you.

1. I think that dealing with the AbstractCharSequence and introducing one
   or more private[lang] methods is not "cutting the chicken a the joints".
   I think one or more methods in AbstractStringBuilder and pattern
   matching, as in the first baselevel of this PR, on those two types
   gives a much more limited & understandable scope of change.

2. I am also beginning to suspect that we, or at least I, have
   the wrong end of the stick on this problem. That is,
   a hypothetical StringBuilder.toArray[Char] is understandable
   and requires fewer change to the code. It "gives out'
   an immutable Array, so that is good. It avoids
   a String allocation with its internal Array allocation
   and copy. It has the equivalent Array copy as the
   later (in the write() path) conversion from String
   to Array[Char]. Net win.

   I think/am_exploring a AbstractStringBuilder having
   a new private[lang] method which takes a function
   which takes one Array[Char] argument. Code
   which wanted to use the StringBuilder/StringBuffer's
   Array value would call the method and pass the function
   they wanted with SB's.value.

   The SB method could decide if it wants to do a
   copy of its Array (value.clone) or pass it to the
   argument function. The other thing is that,
   at least for development, the method could
   call hashCode on the Array after the method
   to check that it had not been altered.

   I/we do not know if all potential users of the
   SB can be done with one argument, but the
   write() case can and can be earning its
   keep while we try to find other candidates
   for improvement.

   Net win here is somewhere between 0 and 1 Array allocation
   and copy.

   So far, I think the promise is in centralizaton of the copy/hand_out_mutable
   decision.

We may have to explore several paths and that can be frustrating. I for one,
encourage, your further work on reducing String allocations. It is removing
sand from the gears.

[david-bouyssie]

I have not lost interest. It is going to take me a few days to review your latest proposal.
I have to get a few things done here. Sorry for introducing a bubble in the pipeline.

Absolutely not an issue, as I'm also very busy this week.

Some thoughts from over the weekend, against the background
of my not having read your latest work in detail. I may be way behind
or just way off. Not to distract you.

Thank you again for your input. I have no issue with a different approach.
I consider this work in progress as a proposal.

I think that dealing with the AbstractCharSequence and introducing one
or more private[lang] methods is not "cutting the chicken a the joints".
I think one or more methods in AbstractStringBuilder and pattern
matching, as in the first baselevel of this PR, on those two types
gives a much more limited & understandable scope of change.

I don't get the suggestion about AbstractCharSequence. Did you meant CharSequence or AbstractStringBuilder?

I am also beginning to suspect that we, or at least I, have
the wrong end of the stick on this problem. That is,
a hypothetical StringBuilder.toArray[Char] is understandable
and requires fewer change to the code. It "gives out'
an immutable Array, so that is good. It avoids
a String allocation with its internal Array allocation
and copy. It has the equivalent Array copy as the
later (in the write() path) conversion from String
to Array[Char]. Net win.

We could eventually use the existing method AbstractStringBuilder.getValue()
https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/lang/AbstractStringBuilder.scala#L14
However, since we need to not use chars that are after the count limit, it's not an option.

The alternative, which is aligned with your proposal, would be to use the existing method StringBuilder/StringBuffer.getChars()
https://docs.oracle.com/javase/8/docs/api/java/lang/StringBuilder.html#getChars-int-int-char:A-int-
https://docs.oracle.com/javase/8/docs/api/java/lang/StringBuffer.html#getChars-int-int-char:A-int-
However this solution involves an extra System.arraycopy() call.

It's true that it would avoid to create a String wrapper, but won't be a zero-copy operation. So we still generate additional garbage that could increase GC pressure (for heavy workloads).

What I like with the CharSequence interface is that:

1. it is very straightforward
   
2. it is implemented by several String related classes

• java.lang -> String, StringBuffer, StringBuilder
• java.nio -> CharBuffer

CharSequence is the common denominator to these different classes.
It is thus to me a very suitable abstraction layer to iterate chars from these entities without additional cost.
I'm still wondering why it has not been adopted in a broader fashion by the JDK authors.

For now, the best solution I came up with, consists in wrapping the CharSequence in a CharBuffer using CharBuffer.wrap(csq, start, end), as shown here:

scala-native/javalib/src/main/scala/java/io/Writer.scala

Line 56 in ff431de

writeCharBuffer(CharBuffer.wrap(csq, start, end))

The main reason of this wrapping, is that CharBuffer is the JDK entity selected as an abstraction layer to deal with character encoding/decoding:
https://docs.oracle.com/javase/8/docs/api/java/nio/charset/CharsetEncoder.html#encodeLoop-java.nio.CharBuffer-java.nio.ByteBuffer-

While I would prefer to have an encodeLoop() method variant specified as encodeLoop(in: CharSequence, out: ByteBuffer), it is the current state of the inherited javalib (which is now based on ScalaJS work).
Re-implementing an encodeLoop() with this alternative signature, would require a significant piece of work, to be sure we don't introduce correctness and performance regressions.
What is not clear to me, is if CharBuffer.wrap(csq, start, end) has any performance drawback, as the allocation could eventually be stack allocated by Interflow (according to a discussion on Discord).

We may have to explore several paths and that can be frustrating. I for one,
encourage, your further work on reducing String allocations. It is removing
sand from the gears.

Let's hope we will find on a solution which is both simple, robust and efficient.

[david-bouyssie]

Hi @LeeTibbert.
When you have a bit of spare time, could you please give me an update?
I think this is a nice step forward, but wise feedback is always good.
Thanks.

[LeeTibbert]

@david-bouyssie Will do. It will have to be tomorrow, Weds.

A lot of discussion has been happening in the String allocation area and I need to
come back up to speed. Sorry to ask an out-of-touch question. Is the code in this PR your current thinking?

[david-bouyssie]

Is the code in this PR your current thinking?

Well, it should. I'm just worried that it is not...
What I mean is that I tried to change the code so that a CharSequence can be encoded with minimal allocation to bytes (in the different kinds of Stream). I wanted to avoid CharSequence -> String conversions when possible.
I couldn't avoid a CharBuffer.wrap call so far, but I guess its cheap compared to String conversion. No numbers so far, though...