-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Custom frame size computation support in Framing #5444
Custom frame size computation support in Framing #5444
Conversation
} | ||
|
||
Parallel.ForEach(GetFutureResults(), async futureResult => | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this out, because introducing parallelism in tests that do not require to execute their asserts in order could save a lot of time.
This commit brought the execution time of each of both tests from 1.2 min
to less than 2s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I wonder where else we could take advantage of this.
An unrelated test in |
|
||
var tempArray = new byte[4].PutInt(unchecked((int)0xFF010203), order: ByteOrder.LittleEndian); | ||
Array.Resize(ref tempArray, 8); | ||
var bs = ByteString.FromBytes(tempArray.PutInt(checked(0x04050607), order: ByteOrder.LittleEndian)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're missing the ByteStringBuilder which would've made this chunk of code more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that was removed in 1.3 or 1.4 and I don't recall why
df48ca5
to
77d5ffe
Compare
77d5ffe
to
a37625b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
||
Parallel.ForEach(GetFutureResults(), async futureResult => | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, I wonder where else we could take advantage of this.
int fieldLength, | ||
ByteOrder byteOrder, | ||
ByteString offset, | ||
ByteString tail) | ||
{ | ||
var h = ByteString.FromBytes(new byte[4].PutInt(payload.Count, order: byteOrder)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the ByteStringBuilder
I bet we could also save on allocations here too...
* Custom frame size computation support in Framing * Speed up of framing spec
) * remove the forced waiting on the underlying transport to start up * racy spec fix: `BackoffOnRestartSupervisor_must_respect_maxNrOfRetries_property_of_OneForOneStrategy` (#5442) issue was that using `AutoReset` could cause a race where if the scheduler could reset the count in-flight and throw off the estimates used by the tests. Using a `ManualReset` avoids this issue. * deleted commented out code * Make updates to `FishUntil` (#5433) * Make updates to `FishUntil` Implement nitpicks from #5430 * Nitpick * cleaned up `FishUntilMessage` * removed `FluentAssertions` reference * fixed bad `TestKit.ReceiveTests` * Custom frame size computation support in Framing (#5444) * Custom frame size computation support in Framing * Speed up of framing spec * optimize `Props` `NewExpression` allocations (#5428) per #5416 (comment) Co-authored-by: Ismael Hamed <1279846+ismaelhamed@users.noreply.github.com>
Original issue: #22129