Skip to content
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

Simplify Heap implementation. #960

Merged
merged 2 commits into from Apr 10, 2019

Conversation

Projects
None yet
2 participants
@Lukasa
Copy link
Contributor

Lukasa commented Apr 9, 2019

Motivation:

Our original Heap implementation used a bunch of static functions
in order to make it clear how it related to textbook heap
implementations. This was primarily done to ensure that it was easier
to see the correctness of the code.

However, we've long been satisfied with the correctness of the code,
and the Heap is one of the best tested parts of NIO. For this reason,
we should be free to refactor it.

Given https://bugs.swift.org/browse/SR-10346, we've been given an
incentive to do that refactor right now. This is because the Heap
is causing repeated CoW operations each time we enqueue new work
onto the event loop. That's a substantial performance cost: well
worth fixing with a small rewrite.

Modifications:

  • Removed all static funcs from Heap
  • Rewrote some into instance funcs
  • Merged the implementation of insert into append.

Result:

Faster Heap, happier users, sadder textbook authors.

@Lukasa Lukasa added this to the 2.0.1 milestone Apr 9, 2019

@Lukasa Lukasa requested a review from weissi Apr 9, 2019

@weissi

weissi approved these changes Apr 9, 2019

Copy link
Member

weissi left a comment

🤘 thanks, looks good to me!

@weissi

This comment has been minimized.

Copy link
Member

weissi commented Apr 9, 2019

@Lukasa would you mind checking if the allocations went down, I'd expect them to for the connection tests :)

@Lukasa Lukasa force-pushed the Lukasa:cb-rewrite-heap branch 3 times, most recently from 931e163 to e898ddd Apr 10, 2019

Simplify Heap implementation.
Motivation:

Our original Heap implementation used a bunch of static functions
in order to make it clear how it related to textbook heap
implementations. This was primarily done to ensure that it was easier
to see the correctness of the code.

However, we've long been satisfied with the correctness of the code,
and the Heap is one of the best tested parts of NIO. For this reason,
we should be free to refactor it.

Given https://bugs.swift.org/browse/SR-10346, we've been given an
incentive to do that refactor right now. This is because the Heap
is causing repeated CoW operations each time we enqueue new work
onto the event loop. That's a substantial performance cost: well
worth fixing with a small rewrite.

Modifications:

- Removed all static funcs from Heap
- Rewrote some into instance funcs
- Merged the implementation of insert into append.
- Added an allocation test to avoid regressing this change.

Result:

Faster Heap, happier users, sadder textbook authors.

@Lukasa Lukasa force-pushed the Lukasa:cb-rewrite-heap branch from e898ddd to f79c7ae Apr 10, 2019

@Lukasa Lukasa merged commit 0179535 into apple:master Apr 10, 2019

1 check passed

pull request validation (5.0) Build finished.
Details

@Lukasa Lukasa deleted the Lukasa:cb-rewrite-heap branch Apr 10, 2019

weissi added a commit that referenced this pull request Apr 12, 2019

Simplify Heap implementation. (#960)
Motivation:

Our original Heap implementation used a bunch of static functions
in order to make it clear how it related to textbook heap
implementations. This was primarily done to ensure that it was easier
to see the correctness of the code.

However, we've long been satisfied with the correctness of the code,
and the Heap is one of the best tested parts of NIO. For this reason,
we should be free to refactor it.

Given https://bugs.swift.org/browse/SR-10346, we've been given an
incentive to do that refactor right now. This is because the Heap
is causing repeated CoW operations each time we enqueue new work
onto the event loop. That's a substantial performance cost: well
worth fixing with a small rewrite.

Modifications:

- Removed all static funcs from Heap
- Rewrote some into instance funcs
- Merged the implementation of insert into append.
- Added an allocation test to avoid regressing this change.

Result:

Faster Heap, happier users, sadder textbook authors.

(cherry picked from commit 0179535)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.