[TO BE DISCUSSED] make generic/closure ByteBuffer methods inlineable #266
Conversation
weissi
changed the title
|
on Linux with Swift 4.0.3 we go from to so also lost 4 allocations. But I reckon @normanmaurer's HTTP1 optimisations should lower that number much more significantly. |
|
@normanmaurer the actual name for That change does make |
|
|
There was a problem hiding this comment.
@moiseev @airspeedswift Is the plan to remove the underscore-prefixed spellings of @inlineable and @usableFromInline once SE-193 lands? Or will those continue to compile?
Sources/NIO/ByteBuffer-core.swift
Outdated
| } | ||
|
|
||
| private func toIndex(_ value: Int) -> Index { | ||
| return Index(truncatingIfNeeded: value) | ||
| @_versioned func toIndex(_ value: Int) -> ByteBuffer.Index { |
There was a problem hiding this comment.
Please do make the @_versioned properties underscore-prefixed.
Motivation: Generics and closures across modules can be problematic as the cause allocations. ByteBuffer is one of NIO's core data types and therefore should be well optimised. Generic/closure taking methods so far were a bit of a problem. And with [SE-0193](https://github.com/apple/swift-evolution/blob/master/proposals/0193-cross-module-inlining-and-specialization.md) accepted, there's not much reason to use those features even though they are (at the moment) @_. For the 1000 requests over 1 connection allocation benchmark we go from "total_allocations": 325378 to "total_allocations": 321366 so we lose 4 allocations per HTTP request/response pair, not super impressive but not nothing either. Modifications: Make generic/closure taking methods on ByteBuffer `@_inlineable` Result: Fewer allocations
weissi
force-pushed
the
jw-make-bb-inlineable
branch
from
ecad819 to
74906b2
Compare
|
thanks @moiseev . Looks better now? |
|
@Lukasa the changes have landed in master. So master branch of Swift right now supports both the old and the new spelling. There are no concrete plans to port it to the 4.2 branch right away. So in order to be compatible, for now, it is safe to stick to the old form of these attributes. |
|
@weissi It's not a matter of how it looks =) I commented primarily because it might enable some other optimizations. |
|
@moiseev Sorry, I should be clearer. Is there any plan to drop these old names, e.g. in Swift 4.2? I'm very reluctant to ship code that we have reason to believe will break unnecessarily in a future release if we can possibly avoid it. |
|
@Lukasa WDYT? I think its "ok" to merge |
|
I don’t think @moiseev has fully answered my question, so I’m still a bit nervous about merging this. |
|
@Lukasa I think we're fine: https://twitter.com/slava_pestov/status/982997370659983360 . Slava implemented this AFAIK |
|
Awesome, thanks. Lemme re-review with that in mind. |
I'm not 100% sure I did exactly the right thing and if we should to this. Paging @airspeedswift and @moiseev .
Motivation:
Generics and closures across modules can be problematic as the cause
allocations. ByteBuffer is one of NIO's core data types and therefore
should be well optimised. Generic/closure taking methods so far were a
bit of a problem. And with
SE-0193
accepted, there's not much reason not to use those features even though they
are (at the moment)
@_.For the 1000 requests over 1 connection allocation benchmark we go from
to
so we lose 4 allocations per HTTP request/response pair, not super
impressive but not nothing either.
Modifications:
Make generic/closure taking methods on ByteBuffer
@_inlineableResult:
Fewer allocations