Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small improvement for String.length #9469

Merged
merged 4 commits into from
Jun 17, 2020

Conversation

abelbraaksma
Copy link
Contributor

@abelbraaksma abelbraaksma commented Jun 17, 2020

The PR removes some overhead for String.length, see #9390 (comment).

The disassembly will change from this:

test      rax,rax              ; rax & rax, in other words: null-check, sets flags
je        short M00_L01        ; if zero (null) jump back to loop
cmp       dword ptr [rax+8],0  ; compare string-length to 0
jbe       short M00_L01        ; if equal, jump back to loop
mov       ecx,[rax+8]          ; if not, get string length fallthrough to loop

to this:

test      rax,rax        ; rax & rax, for null-check, sets flags
je        short M00_L01  ; if zero (null) jump back to loop
mov       ecx,[rax+8]    ; get the string-length from the string and fallthrough to loop

Which, in timings, gives roughly a 33% improvement, but since this is hard to test in isolation, it may be more because more chances exist now for optimizing & inlining this by the JIT (which, in this isolated case, indeed happens).

More info and timings in the connected issue comment above.

@abelbraaksma
Copy link
Contributor Author

Hmm, I've contributed before, though it's a while ago that I made PR's. From time to time you have to sign again?

image

@cartermp
Copy link
Contributor

The CLA is a consequence of moving to dotnet/fsharp I believe?

@abelbraaksma
Copy link
Contributor Author

Possible, anyway, signing took 3 seconds, so it's not a big deal :)

src/fsharp/FSharp.Core/string.fs Outdated Show resolved Hide resolved
src/fsharp/FSharp.Core/string.fs Outdated Show resolved Hide resolved
src/fsharp/FSharp.Core/string.fs Outdated Show resolved Hide resolved
src/fsharp/FSharp.Core/string.fs Outdated Show resolved Hide resolved
@KevinRansom
Copy link
Member

@abelbraaksma It seems as though you combined two changes into one PR and proposed it twice.

/cc @vzarytovskii

@abelbraaksma
Copy link
Contributor Author

@KevinRansom, yeah, that's why I shouldn't do this long after midnight ;)

I meant to split them (@cartermp and @forkiasked for a PR per function), and thought I only locally had done the accidental merge.

I used VS's github integration, and couldn't switch branches on the PR page, so I couldn't create a new PR on the 2nd branch, and after some frustration clearly did something else wrong ;)

Will fix.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@KevinRansom
Copy link
Member

@abelbraaksma thanks for taking care of this.

Kevin

@KevinRansom KevinRansom merged commit 0d1e1d4 into dotnet:master Jun 17, 2020
@abelbraaksma abelbraaksma deleted the String_performance branch June 17, 2020 20:40
baronfel pushed a commit to baronfel/FSharp.Compiler.Service that referenced this pull request Jun 20, 2020
* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Improve performance of String.map"

* Turn String.length into a one-liner, fixes dotnet/fsharp#9469 (comment)
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Improve performance of String.map"

* Turn String.length into a one-liner, fixes dotnet#9469 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants