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.

Sign up for GitHub

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

Jump to bottom

[Level Events] manage level events registration mask #13787

Merged
merged 40 commits into from
Nov 20, 2020
Merged

[Level Events] manage level events registration mask #13787

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7c6055f
Initial implementation for optimized file events
Oct 25, 2020
a5f71d2
format changes
Oct 27, 2020
00f76fc
fix format + make linux run level events to get a bigger sample on CI
Oct 27, 2020
5a5fcf6
improve implementation with new event trigger type
Oct 30, 2020
5b67c40
fix udp, hot restart and proxy listener
Oct 30, 2020
bb201bd
rename function names
Oct 30, 2020
29b9ba4
format changes
Oct 30, 2020
5473250
spelling
Oct 30, 2020
55843d4
fix file event test
Oct 31, 2020
cbb6f81
fix format
Oct 31, 2020
18bb1f2
attempt to fix windows comp
Nov 1, 2020
49597fe
fix formatting
Nov 2, 2020
3795281
fix ssl tests
Nov 2, 2020
b1713b6
format changes
Nov 2, 2020
43ccec1
revert file_event testt
Nov 2, 2020
4ca4422
no need to merge events with mode EVLOOP_ONCE
Nov 2, 2020
804f747
Remove EVLOOP_ONCE from windows + enable event tests
Nov 3, 2020
524826b
format change
Nov 3, 2020
8f450f1
fix simulated_time_system_test
Nov 3, 2020
d5a43f8
ci failures + comments
Nov 3, 2020
9482e16
format changes
Nov 3, 2020
65c1a43
fix asan
Nov 4, 2020
bced31d
small improvements
Nov 6, 2020
c5bb3d0
pr comments
Nov 10, 2020
de29321
handle case with early close and read on activated events
Nov 11, 2020
95de340
fix format
Nov 11, 2020
50ef019
increase coverage
Nov 11, 2020
f7e5741
constexpr guarding
Nov 18, 2020
af5ab9c
fix formatting
Nov 18, 2020
342fe73
fix pr comments
Nov 18, 2020
ee2b6e0
optimize events for winodws
Nov 18, 2020
da34cb0
More constexpr guarding
Nov 18, 2020
54bff41
Merge remote-tracking branch 'upstream/master' into eventsOptimized
Nov 18, 2020
5d135e9
fix merge conflict
Nov 18, 2020
a730507
more constexpr guards vol2
Nov 19, 2020
bc4278d
PR comments by Matt and per file coverage
Nov 19, 2020
afb62cb
spelling
Nov 19, 2020
48f9376
expand comments on event types
Nov 19, 2020
bf30da3
spelling
Nov 19, 2020
3e9ca9c
drop coverage for events to 93.5
Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions include/envoy/event/file_event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ struct FileReadyType {

enum class FileTriggerType { Level, Edge, EmulatedEdge };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of Level now? Windows is the only platform its needed and we fully intend to use the "optimized" flavor anyhow. Or are you using it for testing/comparison?

Copy link
Member Author

Choose a reason for hiding this comment

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

Level events are used by listeners and DNS ( on all platforms), so we cant really remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I did not know that. Have you played with trying to move listeners and DNS to EmulatedEdge? Just curious. This is something that can certainly be done later if there is interest

Copy link
Member

Choose a reason for hiding this comment

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

Can you add some comments now above each trigger type with what they do and what they are used for? I think now it's less clear what all of these do and are for and it would be good to have a comment trail.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a block of comments for EmulatedEdge and some comments with the current use cases of the other event types


static constexpr bool optimizeLevelEvents = true;

// For POSIX developers to get the Windows behavior of file events
// add `#define FORCE_LEVEL_EVENTS` and recompile.
// you need to add the following definitions:
// 1. `FORCE_LEVEL_EVENTS`
// 2. `OPTIMIZE_LEVEL_EVENTS`.
// You can do this with bazel if you add the following build/test options
// `--copt="-DFORCE_LEVEL_EVENTS" --copt="-DOPTIMIZE_LEVEL_EVENTS"`
constexpr FileTriggerType determinePlatformPreferredEventType() {
#if defined(WIN32) || defined(FORCE_LEVEL_EVENTS)
if constexpr (optimizeLevelEvents) {
return FileTriggerType::EmulatedEdge;
} else {
return FileTriggerType::Level;
}
#ifdef OPTIMIZE_LEVEL_EVENTS
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
return FileTriggerType::EmulatedEdge;
#else
return FileTriggerType::Level;
#endif
#else
return FileTriggerType::Edge;
#endif
Expand Down
7 changes: 3 additions & 4 deletions source/common/event/file_event_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,11 @@ FileEventImpl::FileEventImpl(DispatcherImpl& dispatcher, os_fd_t fd, FileReadyCb
// an OOM condition and just crash.
RELEASE_ASSERT(SOCKET_VALID(fd), "");
#ifdef WIN32
RELEASE_ASSERT(trigger_ != FileTriggerType::Edge,
"libevent does not support edge triggers on Windows");
ASSERT(trigger_ != FileTriggerType::Edge, "libevent does not support edge triggers on Windows");
#endif
if constexpr (PlatformDefaultTriggerType != FileTriggerType::EmulatedEdge) {
RELEASE_ASSERT(trigger_ != FileTriggerType::EmulatedEdge,
"Cannot use EmulatedEdge events if they are not the default platform type");
ASSERT(trigger_ != FileTriggerType::EmulatedEdge,
"Cannot use EmulatedEdge events if they are not the default platform type");
}

assignEvents(events, &dispatcher.base());
Expand Down