jdk
8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer
#21730
Open

8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer #21730

mkarg wants to merge 1 commit into openjdk:master from mkarg:getchars
mkarg
mkarg77 days ago (edited 77 days ago)❤ 1

This Pull Request proposes an implementation for [JIRA]: Adding the new method public void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) to the CharSequence interface, providing a bulk-read facility including a default implementation iterating over charAt(int).

In addition, this Pull Request proposes to replace the implementation of Reader.of(CharSequence).read(char[] cbuf, int off, int len) to invoke CharSequence.getChars(next, next + n, cbuf, off) instead of utilizing pattern matching for switch. Also, this PR proposes to implement CharBuffer.getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) as an alias for CharBuffer.get(srcBegin, dst, dstBegin, srcEnd - srcBegin).

To ensure quality...

  • ...the method signature and JavaDocs are adapted from AbstractStringBuilder.getChars(...).
  • ...this PR relies upon the existing tests for Reader.of(CharSequence), as these provide sufficient coverage of all changes introduced by this PR.

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8343111 to be approved

Issues

  • JDK-8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer (Enhancement - P4)
  • JDK-8343111: Add getChars(int, int, char[], int) to CharSequence and CharBuffer (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21730/head:pull/21730
$ git checkout pull/21730

Update a local copy of the PR:
$ git checkout pull/21730
$ git pull https://git.openjdk.org/jdk.git pull/21730/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21730

View PR using the GUI difftool:
$ git pr show -t 21730

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21730.diff

bridgekeeper
bridgekeeper77 days ago

👋 Welcome back mkarg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk
openjdk77 days ago

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk
openjdk77 days ago

@mkarg The following labels will be automatically applied to this pull request:

  • core-libs
  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk openjdk added nio
openjdk openjdk added core-libs
mkarg
mkarg77 days ago

/csr

openjdk openjdk added csr
openjdk
openjdk77 days ago

@mkarg has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mkarg please create a CSR request for issue JDK-8343110 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mkarg
mkarg77 days ago👍 1
robtimus
robtimus commented on 2024-10-26
src/java.base/share/classes/java/lang/CharSequence.java
335 *
336 * @since 24
337 */
338
public default void getChars(int srcBegin, int srcEnd, char[] dst, int dstBegin) {
robtimus77 days ago (edited 77 days ago)👍 2

Shouldn't the getChars methods of String, AbstractStringBuilder, StringBuilder and StringBuffer be marked with @Override?

mkarg77 days ago👍 1

While technically not being necessary, it is definitively a good idea. I will add it to the draft once we actually agreed that we really want to go with this particular signature (see the discussion with Chen).

liach
liach77 days ago😕 1

Sorry for belated mail response, but I think we should design the API to not take source start/end. I think JIT can escape analysis the new String in practice.

mkarg
mkarg77 days ago (edited 77 days ago)

Sorry for belated mail response, but I think we should design the API to not take source start/end. I think JIT can escape analysis the new String in practice.

Chen, thank you for chiming in! There is nothing to be sorry, I was just posting drafts so far! 🙂

Regarding your actual question, I do understand your idea and while originally I had the same in mind (it really is appealing!), I came up with a draft using the original String.getChars() signature instead, due to the following drawbacks:

  • There might exist (possibly lotsof) CharSequence.getChars(int, int, char[], int) implementations already, as this problem (and the idea how to solve it) is anything but new. At least such implementations are String, StringBuilder and StringBuffer. If we come up with a different signature, then none of these already existing performance boosters will get used by Reader.of(CharSequence) automatically - at least until they come up with alias methods. Effectively this leads to (possibly lots) of alias methods. At least it leads to alias methods in String, StringBuilder, StringBuffer and CharBuffer. In contrast, when keeping the signature copied from String.getChars, chances are good that (possibly lots of) implementations will instantly be supported by Reader.of(CharSequence) without alias methods. At least, String, StringBuilder and StringBuffer will be.
  • Since decades people are now very used to StringBuilder.getChars(int, int, char[], int), so (possibly a lot of) people might simply expect us to come up with that lengthy signature. These people might be rather confused (if not to say frustrated) when we now force them to write an intermediate subSequence(int, int) for something that was "such simple" before.
  • Custom implementations of CharSequence.subSequence could come up with the (performance-wise "bad") idea of creating copies instead of views. At least it seems like AbstractStringBuilder is doing that, so chances are "good" that custom libs will do that, too. For example, because they need it for safety. Or possibly, because they have a technical reason that enforces a copy. That would (possibly massively, depending on the actual class) spoil the idea of performance-boosting this PR is actually all about.
liach
liach77 days ago (edited 77 days ago)

Hi Markus, I recommend to continue the discussion on the mailing list instead of on GitHub. Note that GitHub pull requests are only forwarded to the mailing list when it's ready for review, and this proposal is way too early. (And the CSR is too early, too: we should agree on an API surface, such as exception contracts, first)

Sorry that no one has responded to you yet, but many engineers are busy with other areas, such as pushing the JEPs (as you see, we have a huge number of candidate/submitted JEPs right now, and JDK 24 RDP1 is just 6 weeks away). They are monitoring your core-libs thread, stay assured!

mkarg
mkarg77 days ago

@liach Agreed with all you say, but actually: I did not expect any response - in particular on a weekend!

I do not know why apparently people started to review the draft documents. I just filed it for no other purpose than taking note of its existence. 🤔 I thought it is pretty clear to everybody that draft means "do not review". At least this is how I used draft documents in the past 30 years. 🙂

So it seems there was a misunderstanding here regarding my drafts. My intention ever was and still is to first finish the discussion about alternatives (A)...(E) on the core-lib mailing list first. My PR and CSR drafts are intentionally in draft state, which means, they are not open for review yet (there is no rfr label) and solely serve as my public storage for ideas-in-progress in lieu of out-of-band media. The detail discussion you started regarding details of the PR was a bit too early IMHO (and surprised me, and actually your duplicate question in the PR confused me), as it is not even decided that we actually go with proposal A, so why discussing details of exactly that already (my conclusion was, you like to prepare it for the case it becomes the finalist of the mailing list discussion)? Maybe I misunderstood your intent, and you actually wanted to propose alternative (F), then sorry for me not seeing that in the first place.

mkarg Draft: CharSequence.getChars
c56c6f74
mkarg mkarg force pushed from 429b1955 to c56c6f74 76 days ago
bridgekeeper
bridgekeeper21 days ago

@mkarg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

mkarg
mkarg21 days ago

Kindly asking anybody's contribution to the ongoing discussion on the core-devs mailing list. Thanks you.

Login to write a write a comment.

Login via GitHub

Reviewers
Assignees
No one assigned
Labels
Milestone