Skip to content

Conversation

@normanmaurer
Copy link
Member

Motivation:

We stored a closure in the pending array which is not really needed. We can just store the NIOAny directly without creating a closure first.

Modifications:

Replace closure with NIOAny

Result:

Less creation of closures.

Motivation:

We stored a closure in the pending array which is not really needed. We can just store the NIOAny directly without creating a closure first.

Modifications:

Replace closure with NIOAny

Result:

Less creation of closures.
@normanmaurer normanmaurer requested review from Lukasa and weissi April 4, 2018 07:07
@normanmaurer
Copy link
Member Author

noticed this while working on http perf stuff.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Yup, seems reasonable enough to me. I think we expected this to have more functions that needed to be called then we actually ended up getting.

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

👍

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Apr 4, 2018
@Lukasa Lukasa added this to the 1.4.0 milestone Apr 4, 2018
@Lukasa Lukasa merged commit 82578be into apple:master Apr 4, 2018
@weissi
Copy link
Member

weissi commented Apr 4, 2018

@normanmaurer would you mind seeing what this does to the number of allocations we do? Just run

IntegrationTests/run-tests.sh -vf test_01_all

and paste those lines that appear close to the end of (a lot of) output:

1000_reqs_1_conn.remaining_allocations: 0
1000_reqs_1_conn.total_allocations: 325357
DEBUG: [["remaining_allocations": 7, "total_allocations": 325378], ["remaining_allocations": 0, "total_allocations": 325357], ["remaining_allocations": 0, "total_allocations": 325360], ["remaining_allocations": 0, "total_allocations": 325364], ["remaining_allocations": 0, "total_allocations": 325369], ["remaining_allocations": 0, "total_allocations": 325360], ["remaining_allocations": 0, "total_allocations": 325366], ["remaining_allocations": 0, "total_allocations": 325361], ["remaining_allocations": 0, "total_allocations": 325360], ["remaining_allocations": 0, "total_allocations": 325366]]
1_reqs_1000_conn.remaining_allocations: 0
1_reqs_1000_conn.total_allocations: 1686163
DEBUG: [["remaining_allocations": 0, "total_allocations": 1690497], ["remaining_allocations": 0, "total_allocations": 1690400], ["remaining_allocations": 0, "total_allocations": 1689557], ["remaining_allocations": 0, "total_allocations": 1689876], ["remaining_allocations": 0, "total_allocations": 1689258], ["remaining_allocations": 0, "total_allocations": 1689059], ["remaining_allocations": 0, "total_allocations": 1688610], ["remaining_allocations": 0, "total_allocations": 1688442], ["remaining_allocations": 0, "total_allocations": 1686570], ["remaining_allocations": 0, "total_allocations": 1686163]]

the test will also take quite a while to run...

I'd expect this

1000_reqs_1_conn.total_allocations: 325357

to go down quite a bit.

Need to run it before/after your patch obvs.

@normanmaurer
Copy link
Member Author

Before:

1000_reqs_1_conn.total_allocations: 325360

With this pr:

1000_reqs_1_conn.total_allocations: 315350

@normanmaurer normanmaurer deleted the http_pending_nio_any branch April 4, 2018 17:35
@weissi
Copy link
Member

weissi commented Apr 4, 2018

@normanmaurer awesome 👏. That removes 10 allocations per request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants