Add a method for peeking and scanning bytes as integers #89

tenderlove · 2024-02-22T23:13:02Z

This commit adds scan_byte and peek_byte. scan_byte will scan the current byte, return it as an integer, and advance the cursor. peek_byte will return the current byte as an integer without advancing the cursor.

Currently StringScanner#get_byte returns a string, but I want to get the current byte without allocating a string. I think this will help with writing high performance lexers.

Thanks!

kou

I think that #read_byte and #peek_byte are better because #getbyte is obsoleted by #get_byte.

BTW, #read_byte may not a good name... I don't feel that it implies that integer not string... Do you have other candidate name? (I'll also consider better name.)

tenderlove · 2024-02-22T23:41:27Z

I think that #read_byte and #peek_byte are better because #getbyte is obsoleted by #get_byte.

Adding an underscore seems fine. I chose readbyte because of IO#readbyte:

>> File.open("Rakefile").readbyte
=> 114
>> File.open("Rakefile").getbyte
=> 114
>>

BTW, #read_byte may not a good name... I don't feel that it implies that integer not string... Do you have other candidate name? (I'll also consider better name.)

I'm not sure what to name it. I think String#getbyte returned a string in the Ruby 1.8 era, but since Ruby 1.9, it returned an integer.

Maybe getchi? Like getch, but returns an int? I'm not sure 😅

kou

+1

I chose readbyte because of IO#readbyte.

Ah, OK. I didn't notice it. It makes sense. Let's use read (read_byte) for this feature.

We may want to deprecate get_byte and getbyte.

Could you update these lists?

strscan/ext/strscan/strscan.c

Lines 1604 to 1619 in 338d870

    
            * === Advancing the Scan Pointer 
        
            * 
        
            * - #getch 
        
            * - #get_byte 
        
            * - #scan 
        
            * - #scan_until 
        
            * - #skip 
        
            * - #skip_until 
        
            * 
        
            * === Looking Ahead 
        
            * 
        
            * - #check 
        
            * - #check_until 
        
            * - #exist? 
        
            * - #match? 
        
            * - #peek

Could you also update the PR description because we use the PR description for commit message?

@headius Could you review JRuby part?

test/strscan/test_stringscanner.rb

ext/strscan/strscan.c

kou · 2024-02-23T07:21:45Z

run-test.rb

@@ -1,5 +1,6 @@
 #!/usr/bin/env ruby

+gem 'strscan'


Wow! I didn't notice that we need this to use installed gem with Ruby 3.2 and 3.3.

We might be able to use bundle exec? I'm not sure what the policy of bundled gems is. I thought you didn't need gem either, but it seems to fix tests in CI.

tenderlove · 2024-02-23T17:34:41Z

I chose readbyte because of IO#readbyte.

Ah, OK. I didn't notice it. It makes sense. Let's use read (read_byte) for this feature.

We may want to deprecate get_byte and getbyte.

I kind of wish getbyte returns an int so that StringScanner API was similar to IO and String (so that people don't need to learn new method names). But changing the return value seems to introduce an incompatibility. Maybe we could do strscan version 4.0.0 and change the return value?

Could you update these lists?

Fixed!

Could you also update the PR description because we use the PR description for commit message?

Done!

kou · 2024-02-24T00:30:46Z

I can understand what you said. I will reconsider API. Please give me a few days...

kou · 2024-02-25T07:22:13Z

How about using scan_byte instead of readbyte/read_byte?
Because:

IO#readbyte raises EOFError for EOF case but we don't want to do it.
StringScanner isn't an IO-like object. So read* is a bit strange.
We have scan/scan_until that advance the scan pointer. Adding scan_byte to the scan family will not be strange.

I can understand the same getbyte method name may be easy to guess but I don't want to introduce a backward incompatibility without migration period. (StringScanner isn't IO-like/String-like object. So IO/String like method name may not be suitable for StringScanner.)

tenderlove · 2024-02-25T20:58:50Z

How about using scan_byte instead of readbyte/read_byte?

Yes! I like this a lot. I'll change the method name.

Because:

IO#readbyte raises EOFError for EOF case but we don't want to do it.

StringScanner isn't an IO-like object. So read* is a bit strange.

We have scan/scan_until that advance the scan pointer. Adding scan_byte to the scan family will not be strange.

💯

This commit adds `scan_byte` and `peek_byte`. `scan_byte` will read the current byte, return it as an integer, and advance the cursor. `peek_byte` will return the current byte as an integer without advancing the cursor.

Co-authored-by: Sutou Kouhei <kou@clear-code.com>

kou · 2024-02-26T00:44:58Z

Let's merge this.

@headius If you have any comment, please let us know later.

integers (ruby/strscan#89) This commit adds `scan_byte` and `peek_byte`. `scan_byte` will scan the current byte, return it as an integer, and advance the cursor. `peek_byte` will return the current byte as an integer without advancing the cursor. Currently `StringScanner#get_byte` returns a string, but I want to get the current byte without allocating a string. I think this will help with writing high performance lexers. --------- ruby/strscan@873aba2e5d Co-authored-by: Sutou Kouhei <kou@clear-code.com>

headius · 2024-02-26T17:02:32Z

I was unable to review in time but there are some performance improvements I will submit as a separate PR.

headius · 2024-02-26T17:32:08Z

The PR in #90 optimizes the new methods and should be merged before release. The original was functionally correct but str.getBytes() returns a copy of the byte range, not a pointer to the actual byte range within the string (because we COW and JVM can't return a pointer to an array offset, use ByteList#get to get the byte at its correct offset).

A few simple changes on top of #89: * Fields accessed more than once should be loaded into a local variable (curr, byteList, runtime). Otherwise, the JVM in some modes will re-load from memory due to the possibility of a concurrent update (better caching, constant propagation). * Use ByteList.get(index) to access directly rather than str.getBytes which makes a copy byte[] (reduce allocation, let ByteList calculate begin offset and 0xFF masking). Performance is nearly 2x better, mostly because we've eliminated the copy: BEFORE: ``` [] strscan $ jruby -Xcompile.invokedynamic -Ilib -rstrscan -rbenchmark -e 'ss = StringScanner.new("x"); 10.times { puts Benchmark.measure { i = 0; while i < 100_000_000; i+=1; ss.peek_byte; end } }' 1.870000 0.080000 1.950000 ( 1.393004) 1.300000 0.020000 1.320000 ( 1.221120) 1.210000 0.000000 1.210000 ( 1.201006) 1.220000 0.010000 1.230000 ( 1.188845) 1.240000 0.000000 1.240000 ( 1.226052) 1.230000 0.010000 1.240000 ( 1.203179) 1.490000 0.000000 1.490000 ( 1.254489) 1.230000 0.010000 1.240000 ( 1.218346) 1.230000 0.000000 1.230000 ( 1.206789) 1.300000 0.010000 1.310000 ( 1.237078) ``` AFTER: ``` [] strscan $ jruby -Xcompile.invokedynamic -Ilib -rstrscan -rbenchmark -e 'ss = StringScanner.new("x"); 10.times { puts Benchmark.measure { i = 0; while i < 100_000_000; i+=1; ss.peek_byte; end } }' 1.250000 0.080000 1.330000 ( 0.937111) 0.740000 0.000000 0.740000 ( 0.707315) 0.710000 0.010000 0.720000 ( 0.701804) 0.710000 0.000000 0.710000 ( 0.697933) 0.710000 0.000000 0.710000 ( 0.704491) 0.730000 0.010000 0.740000 ( 0.712514) 0.710000 0.000000 0.710000 ( 0.701410) 0.740000 0.000000 0.740000 ( 0.715182) 0.720000 0.010000 0.730000 ( 0.703247) 0.770000 0.000000 0.770000 ( 0.768175) ```

…#91) The methods were added in #89 but they aren't implemented in TruffleRuby yet. So let's omit them for now to have CI green.

tenderlove requested a review from kou February 22, 2024 23:13

kou reviewed Feb 22, 2024

View reviewed changes

tenderlove force-pushed the readbyte branch 2 times, most recently from 55cde40 to 1e8ec46 Compare February 22, 2024 23:58

kou approved these changes Feb 23, 2024

View reviewed changes

kou requested a review from headius February 23, 2024 07:21

tenderlove and others added 3 commits February 25, 2024 15:26

Add a method for peeking and reading bytes as integers

7de4ff5

This commit adds `scan_byte` and `peek_byte`. `scan_byte` will read the current byte, return it as an integer, and advance the cursor. `peek_byte` will return the current byte as an integer without advancing the cursor.

Update ext/strscan/strscan.c

59436e1

Co-authored-by: Sutou Kouhei <kou@clear-code.com>

Update test/strscan/test_stringscanner.rb

e035b4d

Co-authored-by: Sutou Kouhei <kou@clear-code.com>

tenderlove force-pushed the readbyte branch from f3ea198 to e035b4d Compare February 25, 2024 23:27

kou changed the title ~~Add a method for peeking and reading bytes as integers~~ Add a method for peeking and scanning bytes as integers Feb 26, 2024

kou merged commit 873aba2 into master Feb 26, 2024

kou deleted the readbyte branch February 26, 2024 00:45

headius mentioned this pull request Feb 26, 2024

Optimize scan_byte and peek_byte #90

Merged

andrykonchin mentioned this pull request Mar 26, 2024

Omit tests for #scan_byte and #peek_byte on TruffleRuby temporary #91

Merged

kou pushed a commit that referenced this pull request Mar 27, 2024

Omit tests for #scan_byte and #peek_byte on TruffleRuby temporary (…

844d963

…#91) The methods were added in #89 but they aren't implemented in TruffleRuby yet. So let's omit them for now to have CI green.

gettalong mentioned this pull request Nov 7, 2024

Provide an efficient way to parse Integers (and Floats)? #113

Closed

eregon mentioned this pull request Feb 10, 2025

truffleruby fails sinatra tests (liquid) NoMethodError: undefined method 'peek_byte' for #<StringScanner ...> truffleruby/truffleruby#3788

Closed

	* === Advancing the Scan Pointer
	*
	* - #getch
	* - #get_byte
	* - #scan
	* - #scan_until
	* - #skip
	* - #skip_until
	*
	* === Looking Ahead
	*
	* - #check
	* - #check_until
	* - #exist?
	* - #match?
	* - #peek

Add a method for peeking and scanning bytes as integers #89

Add a method for peeking and scanning bytes as integers #89

Uh oh!

Conversation

tenderlove commented Feb 22, 2024 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

tenderlove commented Feb 22, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kou Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

tenderlove Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

tenderlove commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou commented Feb 24, 2024

Uh oh!

kou commented Feb 25, 2024

Uh oh!

tenderlove commented Feb 25, 2024

Uh oh!

kou commented Feb 26, 2024

Uh oh!

headius commented Feb 26, 2024

Uh oh!

headius commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tenderlove commented Feb 22, 2024 •

edited by kou

Loading

tenderlove commented Feb 23, 2024 •

edited

Loading