Skip to content

String mapi ~2.5x perf improvement (v2) #9482

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

Merged
merged 10 commits into from
Jul 25, 2020

Conversation

abelbraaksma
Copy link
Contributor

@abelbraaksma abelbraaksma commented Jun 17, 2020

See for details of timings that motivated this improvement: #9390 (comment).

TLDR: using a char [] improves ~2.1x, then 2x unrolling brings this to ~2.5x (edit: no unrolling will be done, the JIT should do that). In other words, if it took 1 second before, it now takes 0.4 seconds.

Dependent on: #9481.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jun 18, 2020

Hmm, CI errors:

at NUnit.Framework.Assert.AreEqual(Object expected, Object actual, String message) in /home/vsts/work/1/s/tests/FSharp.Core.UnitTests/NUnitFrameworkShims.fs:line 93
at FSharp.Core.UnitTests.Collections.StringModule.MapI() in /home/vsts/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs:line 85

That's this test:

[<Test>]
member this.MapI() =
    let e1 = String.mapi (fun i c -> char(int c + i)) "foo"
    Assert.AreEqual("fpq", e1)

    let e2 = String.mapi (fun i c -> c) null 
    Assert.AreEqual("", e2)

And yes, these were wrong, incorrectly copied & changed the code from the BDN project. I've added more tests since we're now unrolling by % 2, which warranted adding the corners for 0, 1, 2 and I've added a few general ones to ensure the mapping function isn't called when it shouldn't: 4d15697

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Jun 18, 2020

Another sourcelink fail?

C:\Users\VssAdministrator.nuget\packages\microsoft.sourcelink.common\1.1.0-beta-20206-02\build\InitializeSourceControlInformation.targets(3,81): error MSB4022: The result "" of evaluating the value "$(_MicrosoftSourceLinkCommonAssemblyFile)" of the "AssemblyFile" attribute in element <UsingTask> is not valid.

Not sure what causes this, doesn't seem related.

@abelbraaksma
Copy link
Contributor Author

All's green now, newly added tests passed.

@abelbraaksma
Copy link
Contributor Author

@KevinRansom, the unrolling was removed, as discussed, all requested changes implemented and green. Good to go?

@abelbraaksma abelbraaksma requested a review from KevinRansom June 24, 2020 23:54
@abelbraaksma
Copy link
Contributor Author

@cartermp, you merged master into this, I think this is now ready?

@cartermp
Copy link
Contributor

cartermp commented Jul 5, 2020

This will require a re-review by @KevinRansom to merge

@abelbraaksma
Copy link
Contributor Author

/cc @KevinRansom

@cartermp cartermp merged commit b3c3ad7 into dotnet:master Jul 25, 2020
@abelbraaksma
Copy link
Contributor Author

Thanks @KevinRansom!

@abelbraaksma abelbraaksma deleted the String.mapi-perf branch July 25, 2020 20:35
@KevinRansom
Copy link
Contributor

@abelbraaksma , happy to be of service.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Move `String.length` to the top of its module so that the `length` function is in scope

* Using known length for mapping, and unrolling x2 to improve perf of String.mapi by 2.5x

* Fix two unapparent errors in the unroll loop, fix nit on parens

* Add String.mapi tests for all new unrolling corner cases, and some general ones

* Add side-effect test, this would fail if sequentiality of String.mapi is broken

* Remove "foo" from tests

* Remove unrolling of loop in String.mapi

Co-authored-by: Phillip Carter <pcarter@fastmail.com>
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.

3 participants