-
Notifications
You must be signed in to change notification settings - Fork 825
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
Conversation
…nction is in scope
…tring.mapi by 2.5x
Hmm, CI errors:
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 |
Another sourcelink fail?
Not sure what causes this, doesn't seem related. |
All's green now, newly added tests passed. |
@KevinRansom, the unrolling was removed, as discussed, all requested changes implemented and green. Good to go? |
@cartermp, you merged master into this, I think this is now ready? |
This will require a re-review by @KevinRansom to merge |
/cc @KevinRansom |
…harp into String.mapi-perf
Thanks @KevinRansom! |
@abelbraaksma , happy to be of service. |
* 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>
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.