prelim code to test sys_read loop replace by khwilliamson · Pull Request #24521 · Perl/perl5

khwilliamson · 2026-07-01T19:39:35Z

This set of changes does not require a perldelta entry.

khwilliamson · 2026-07-01T22:40:53Z

sorry, should be fixed now

khwilliamson · 2026-07-02T12:41:14Z

Just to be clear. I'm not saying this patch should be used instead of your proposal; I'm saying this should be used regardless of your proposal for those cases where yours would not kick in.

The 12 where this kicks in was determined with just a bit of experimentation; it could be tweaked. I did try a quick and dirty check of changing it to 1, and the entire test suite seems to run 1% faster, but that could be noise. I ran a few trials, and the changed version never ran slower.

khwilliamson · 2026-07-02T12:52:04Z

sys_read() when it gets a partial character retries asking for just enough bytes to complete that character. Then it loops to ask for any remaining length the caller has asked for. The code could be simplified if it just always asked for the entire remaining desired length. @Leont, Is there as reason it works this way?

khwilliamson · 2026-07-02T15:11:36Z

I see that S_sv_gets_read_record() in sv.c has similar logic. I don't know if it gets long enough data to justify changing it. @tonycoz any opinions?

Leont · 2026-07-02T16:15:30Z

sys_read() when it gets a partial character retries asking for just enough bytes to complete that character. Then it loops to ask for any remaining length the caller has asked for. The code could be simplified if it just always asked for the entire remaining desired length. @Leont, Is there as reason it works this way?

I can't imagine any good reason for it working that way TBH.

chansen · 2026-07-02T17:20:00Z

@khwilliamson, I hadn't seen your comments here until now. I've updated #24511 with the benchmark results from this PR.

chansen · 2026-07-03T08:32:21Z

sys_read() when it gets a partial character retries asking for just enough bytes to complete that character. Then it loops to ask for any remaining length the caller has asked for. The code could be simplified if it just always asked for the entire remaining desired length. @Leont, Is there as reason it works this way?

The partial-completion request and the main request stop on different conditions, so they can't be merged into one "ask for everything remaining" call.

The main loop uses count == length to detect short reads / EOF: a read returning fewer bytes than asked means the stream is drained, so the loop exits. The partial-completion read is the opposite case; it must keep going on a short read, because stopping there would leave a truncated character in the buffer.

Requesting exactly the missing continuation bytes also keeps every read character-aligned, which is what makes the count == length short-read check valid in the first place.

On seekable files this is redundant (you could over-read and rewind). It matters on pipes/sockets/ttys, where over-reading either blocks waiting for bytes the caller never asked for, or consumes bytes belonging to the next logical read that can't be pushed back.

khwilliamson · 2026-07-04T23:31:51Z

I'm not convinced that there isn't a way around these. Yes a partial can never be returned and hence has to have special case code that is byte-oriented,, but that doesn't mean we can't ask for more data from the read than just enough to complete the partial.

I currently have some puzzlements.

The documentation and implementation of pp_recv() indicate that it dies when called on a :utf8 handle. Yet there are lines below that do if (fp_utf8) SvUTF8_on(bufsv); I'm thinking that conditional is always false.
I don't understand how things get turned into characters from bytes. I think that the input length for pp_read is interpreted as characters, and that the code assumes every character to be read is a single byte. On return it counts the characters returned, and if there were fewer characters than bytes, it loops to read again, still assuming everything to be read is a single byte. Rinse, repeat, until the whole number of requested characters get read. This implies that the number of reads is proportional to the average byte length of characters. I don't see a way around this that wouldn't have the chance of requesting more bytes than the caller wanted, maybe hanging.
This doesn't make sense to me:

    /* Allocating length + offset + 1 isn't perfect in the case of reading
       bytes from a byte file handle into a UTF8 buffer, but it won't harm us
       unduly.
       (should be 2 * length + offset + 1, or possibly something longer if
       IN_ENCODING Is true) */
    buffer  = SvGROW(bufsv, (STRLEN)(length+offset+1));

For one this comment is the only place I found where the symbol IN_ENCODING appears. Second, we know if the file handle is byte or not, so why not assume the worst case for allocation, instead of potentially doing a bunch of 'SVGROW's. Or is the worst case potentially too large?

khwilliamson · 2026-07-05T15:42:12Z

@chansen I did get it to work to request more at a time than the rest of the continuation bytes for the partial character. This should cut down on the number of read calls significantly. If it is convenient, could you test the new version to see how much difference in time it makes

chansen · 2026-07-05T18:36:08Z

@chansen I did get it to work to request more at a time than the rest of the continuation bytes for the partial character. This should cut down on the number of read calls significantly. If it is convenient, could you test the new version to see how much difference in time it makes

Thanks, I'll benchmark it. First though, my sanity check caught a correctness regression with the new version: a plain

read $fh, my $text, $LENGTH  

open(my $fh, '<:encoding(UTF-8)', 'corpus/data/ar.txt')  
  or die qq/open: '$!'/;  
read($fh, my $buf, 14308)  
  or die qq/read: '$!'/;  
say length $buf;   # 14306, expected 14308

ar.txt is a valid file of exactly 14308 code points, so this is dropping the last characters rather than looping to fulfil the count.

corpus/ar.txt

chansen · 2026-07-06T21:44:38Z

There seems to be an issue with the recent changes. I had to disable all sanity length checks because they were reporting incorrect lengths, and after that I encountered a segmentation fault (core dumped). That said, the numbers before the crash looked promising.

ar.txt: 25 KiB; 14K code points; 1.81 units/point
  U+0000..U+007F                3K  18.9%
  U+0080..U+07FF               12K  81.1%
  scalar:encoding(UTF-8)         534 MB/s
  scalar:utf8_strict            3676 MB/s  (6.89x)
  scalar:utf8                  10997 MB/s  (20.61x)
  scalar                       24761 MB/s  (46.40x)

el.txt: 102 KiB; 59K code points; 1.77 units/point
  U+0000..U+007F               14K  23.1%
  U+0080..U+07FF               45K  76.9%
  U+0800..U+FFFF                38   0.1%
  scalar:encoding(UTF-8)         529 MB/s
  scalar:utf8_strict            4178 MB/s  (7.90x)
  scalar:utf8                  15776 MB/s  (29.83x)
  scalar                       44181 MB/s  (83.54x)

en.txt: 80 KiB; 82K code points; 1.00 units/point
  U+0000..U+007F               82K  99.9%
  U+0080..U+07FF                18   0.0%
  U+0800..U+FFFF                49   0.1%
  scalar:encoding(UTF-8)        1743 MB/s
  scalar:utf8_strict            4312 MB/s  (2.47x)
  scalar:utf8                  16605 MB/s  (9.53x)
  scalar                       39552 MB/s  (22.69x)

ja.txt: 176 KiB; 65K code points; 2.79 units/point
  U+0000..U+007F                7K  10.7%
  U+0080..U+07FF                30   0.0%
  U+0800..U+FFFF               58K  89.3%
Segmentation fault (core dumped)

khwilliamson · 2026-07-06T21:53:02Z

Sorry. I was just finishing up fixes. This now passes the arabic test script. I think the changes will correct your other issues as well.

khwilliamson · 2026-07-06T21:57:39Z

Also 756c3a0 reproduces some of the cachegrind numbers that I got when I added the per-word looping in 2019

chansen · 2026-07-09T11:25:36Z

The throughput is higher than in the previous results.

ar.txt: 25 KiB; 14K code points; 1.81 units/point
  U+0000..U+007F                3K  18.9%
  U+0080..U+07FF               12K  81.1%
  scalar:encoding(UTF-8)         535 MB/s
  scalar:utf8_strict            3287 MB/s  (6.14x)
  scalar:utf8                   7789 MB/s  (14.55x)
  scalar                       22825 MB/s  (42.64x)

el.txt: 102 KiB; 59K code points; 1.77 units/point
  U+0000..U+007F               14K  23.1%
  U+0080..U+07FF               45K  76.9%
  U+0800..U+FFFF                38   0.1%
  scalar:encoding(UTF-8)         512 MB/s
  scalar:utf8_strict            3621 MB/s  (7.07x)
  scalar:utf8                   9965 MB/s  (19.46x)
  scalar                       36969 MB/s  (72.21x)

en.txt: 80 KiB; 82K code points; 1.00 units/point
  U+0000..U+007F               82K  99.9%
  U+0080..U+07FF                18   0.0%
  U+0800..U+FFFF                49   0.1%
  scalar:encoding(UTF-8)        1640 MB/s
  scalar:utf8_strict            3736 MB/s  (2.28x)
  scalar:utf8                   9660 MB/s  (5.89x)
  scalar                       37044 MB/s  (22.59x)

ja.txt: 176 KiB; 65K code points; 2.79 units/point
  U+0000..U+007F                7K  10.7%
  U+0080..U+07FF                30   0.0%
  U+0800..U+FFFF               58K  89.3%
  scalar:encoding(UTF-8)         950 MB/s
  scalar:utf8_strict            3625 MB/s  (3.82x)
  scalar:utf8                  10351 MB/s  (10.90x)
  scalar                       48301 MB/s  (50.85x)

lv.txt: 135 KiB; 127K code points; 1.09 units/point
  U+0000..U+007F              117K  92.0%
  U+0080..U+07FF                9K   7.1%
  U+0800..U+FFFF                1K   0.9%
  scalar:encoding(UTF-8)         558 MB/s
  scalar:utf8_strict            3706 MB/s  (6.64x)
  scalar:utf8                  10559 MB/s  (18.91x)
  scalar                       45277 MB/s  (81.09x)

ru.txt: 148 KiB; 85K code points; 1.78 units/point
  U+0000..U+007F               19K  22.6%
  U+0080..U+07FF               66K  77.0%
  U+0800..U+FFFF               364   0.4%
  scalar:encoding(UTF-8)         515 MB/s
  scalar:utf8_strict            3688 MB/s  (7.16x)
  scalar:utf8                  10473 MB/s  (20.33x)
  scalar                       42231 MB/s  (81.98x)

sv.txt: 94 KiB; 93K code points; 1.04 units/point
  U+0000..U+007F               90K  96.4%
  U+0080..U+07FF                3K   3.5%
  U+0800..U+FFFF               171   0.2%
  scalar:encoding(UTF-8)         710 MB/s
  scalar:utf8_strict            3641 MB/s  (5.13x)
  scalar:utf8                  10440 MB/s  (14.71x)
  scalar                       39119 MB/s  (55.12x)

I rewrote this function in 2019 via 71d63d0 to do advancing per-word versus by character for long enough strings, thus saving significant amounts of execution time. FYI, at the bottom is a copy of some of the data presented in that commit message. The function traditionally did just a bit of syntax error checking of the parsed string. I chose to not add any, since the goal was speed, but also chose to not take any away, so that it would behave the same as always. That caused a tiny bit of slowdown than if checking were eliminated. This commit adds a new check at the beginning of the function in the form of an assert (active only in DEBUG builds) that the input string is positioned at the beginning of a character. This should have been a no-brainer, adding essentially nothing and verifying an important sanity criterium ------------------------- Data from 2019 commit Very long strings run an order of magnitude fewer instructions than blead. Here are worst case scenarios (7 bytes after word boundary). The values are in terms of percent, with blead (at the time) always set to 100. Numbers above 100 are good; below bad string length 10000000 characters; 1 bytes per character blead patch ------ ------- Ir 100.00 814.53 Dr 100.00 1069.58 Dw 100.00 3296.55 COND 100.00 1575.83 IND 100.00 100.00 string length 5000000 characters; 2 bytes per character blead patch ------ ------- Ir 100.00 408.86 Dr 100.00 536.32 Dw 100.00 1698.31 COND 100.00 788.72 IND 100.00 100.00 string length 3333333 characters; 3 bytes per character blead patch ------ ------- Ir 100.00 273.64 Dr 100.00 358.56 Dw 100.00 1165.55 COND 100.00 526.35 IND 100.00 100.00 string length 2500000 characters; 4 bytes per character blead patch ------ ------- Ir 100.00 206.03 Dr 100.00 269.68 Dw 100.00 899.17 COND 100.00 395.17 IND 100.00 100.00

'len' is more associated with number of bytes; this is the number of characters, so use 'count'

…ormed input The code works correctly if the input is well-formed. But it was returning a count of the bytes instead of characters for malformed input. It raised a 'utf8' warning, if enabled, before the bad return.

A future commit will surround the indented lines with a block, and will remove a block from the outdented ones. This commit leaves things in an apparent inconsistent state, but this will be fixed the commit after the next

The next commit would otherwise potentially cause malformed input to read off the end of the buffer.

This is in preparation for the next commit to simplify things

When nearing the end of the input string when counting continuations per-word, we switch to counting per-character, using UTF8SKIP. Prior to this commit, we counted continuations based on UTF8SKIP and later converted to characters. But it is simpler to convert the continuations count so far to a character count, and simply use that from then on

Instead of breaking out of the loop when we find an error go directly to the error-handling code. This skips an extra conditional after the loop for this case.

This is in preparation for the next commit

Previous commits in this series have made these two areas identical except for comments, and have repositioned things so that they can easily be consolidated.

This is in preparation for more possible types of warning messages

Reading this code, I don't think the condition tested for by the 'if' can happen, but I added an assert in case I'm wrong.

It is illegal to call this function with the start pointer beyond the end pointer. This is asserted against in PERL_ARGS_ASSERT_UTF8_LENGTH. Therefore no additional code is needed to check against that.

This is to enable this to still work on C++ after a future commit would otherwise fail to compile because of goto crossing initialization

This is in preparation for future commits

This is in preparation for it being needed again.

This function is like utf8_length, but has hooks for code that is reading from a non-rewindable byte stream to more easily handle input stopping in the middle of multi-byte characters. The function is marked API, but experimental.

This code is used for 3 different ops. In the non-pp_read ones, the values are in bytes; in pp_read, they can be in characters as well. I found the names confusing given this intermixing. Changing the names of the two variables that always refer to character values helps lessen the confusion Also add a few clarifying comments

This results in significant speed improvements. See Perl#24511

This is when $/ is set to an integer reference. This speeds up the process greatly, and simplifies the code here.

khwilliamson · 2026-07-10T12:42:00Z

I have polished this up, so I think it would be ready for merging if not for needing some APItests, which I'm holding off on until that gets split.

@tonycoz, this now has changed sv_gets_read_record to use the new utf8_length_maybe_partial

chansen · 2026-07-10T16:00:28Z

@khwilliamson Nice work!

khwilliamson changed the title ~~pp_sys.c: Wrap comment to fit in 80 columns~~ prelim code to test sys_read loop replace Jul 1, 2026

khwilliamson force-pushed the sys_read branch from 6a6f03e to 4f2b37b Compare July 1, 2026 22:40

khwilliamson force-pushed the sys_read branch from 4f2b37b to 81f2ab4 Compare July 5, 2026 15:36

khwilliamson force-pushed the sys_read branch from 81f2ab4 to d31966d Compare July 6, 2026 21:51

khwilliamson force-pushed the sys_read branch from d31966d to 5adc867 Compare July 9, 2026 23:20

khwilliamson added 11 commits July 9, 2026 17:24

utf8_length(): Change variable name

b503024

'len' is more associated with number of bytes; this is the number of characters, so use 'count'

utf8_length: White space only.

3545706

A future commit will surround the indented lines with a block, and will remove a block from the outdented ones. This commit leaves things in an apparent inconsistent state, but this will be fixed the commit after the next

utf8_length: Change "do { } while" to plain while

4657c56

The next commit would otherwise potentially cause malformed input to read off the end of the buffer.

utf8_length: Split a loop into two.

1acecfd

This is in preparation for the next commit to simplify things

utf8_length: Avoid an extra check

6ac7e19

Instead of breaking out of the loop when we find an error go directly to the error-handling code. This skips an extra conditional after the loop for this case.

utf8_length: Move error handling code to function end

1f71aed

This is in preparation for the next commit

utf8_length: Consolidate two areas of code

5012c1c

Previous commits in this series have made these two areas identical except for comments, and have repositioned things so that they can easily be consolidated.

utf8_length: Generalize warning code

b718d27

This is in preparation for more possible types of warning messages

khwilliamson added 10 commits July 9, 2026 17:27

utf8_length: Replace 'if' by assert()

7342d2a

Reading this code, I don't think the condition tested for by the 'if' can happen, but I added an assert in case I'm wrong.

utf8_length: Remove redundant check

c95b6ed

It is illegal to call this function with the start pointer beyond the end pointer. This is asserted against in PERL_ARGS_ASSERT_UTF8_LENGTH. Therefore no additional code is needed to check against that.

utf8_length: Move declaration

6c72eab

This is to enable this to still work on C++ after a future commit would otherwise fail to compile because of goto crossing initialization

utf8_length: Move assert

3f941a3

This is in preparation for future commits

utf8_length: Store expression result in a variable

2c3e60f

This is in preparation for it being needed again.

Create utf8_length_maybe_partial()

a75a3fe

This function is like utf8_length, but has hooks for code that is reading from a non-rewindable byte stream to more easily handle input stopping in the middle of multi-byte characters. The function is marked API, but experimental.

pp_sys.c: Wrap comment to fit in 80 columns

a44cc80

pp_read: Use utf8_length_maybe_partial

02accb8

This results in significant speed improvements. See Perl#24511

sv_gets_read_record: Use utf8_length_maybe_partial

b58cfae

This is when $/ is set to an integer reference. This speeds up the process greatly, and simplifies the code here.

khwilliamson force-pushed the sys_read branch from 5adc867 to b58cfae Compare July 9, 2026 23:31

khwilliamson marked this pull request as ready for review July 10, 2026 12:40

github-actions Bot added the hasConflicts label Jul 16, 2026

Uh oh!

Conversation

khwilliamson commented Jul 1, 2026

Uh oh!

khwilliamson commented Jul 1, 2026

Uh oh!

khwilliamson commented Jul 2, 2026

Uh oh!

khwilliamson commented Jul 2, 2026

Uh oh!

khwilliamson commented Jul 2, 2026

Uh oh!

Leont commented Jul 2, 2026

Uh oh!

chansen commented Jul 2, 2026

Uh oh!

chansen commented Jul 3, 2026

Uh oh!

khwilliamson commented Jul 4, 2026

Uh oh!

khwilliamson commented Jul 5, 2026

Uh oh!

chansen commented Jul 5, 2026

corpus/ar.txt

Uh oh!

chansen commented Jul 6, 2026

Uh oh!

khwilliamson commented Jul 6, 2026

Uh oh!

khwilliamson commented Jul 6, 2026

Uh oh!

chansen commented Jul 9, 2026

Uh oh!

khwilliamson commented Jul 10, 2026

Uh oh!

chansen commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants