Extract common Mask/Dart_Port settings of linux event handler implementation to eventhandler.h

Extract common Mask/Dart_Port settings of linux event handler implementation to eventhandler.h

R=sgjesse@google.com, wibling@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43585

[kustermann, 2015-02-06 13:40:21]

kustermann@google.com changed reviewers:
+ sgjesse@google.com, wibling@google.com

[kustermann, 2015-02-06 13:40:21]

Incorporated gustav's changes.

If you have ideas of better handling this duplication of tokens/masks/... let me
know.

[wibling, 2015-02-06 14:09:28]

lgtm

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandler.h
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:197: if (head_ != NULL && head_->next_ != NULL)
{
NIT: you could assert head_->next != NULL inside the if and not check.

[Søren Gjesse, 2015-02-06 14:42:18]

LGTM

I like all the asserts you have added.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandler.h
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:246: // Returns the bit-wise OR of events t
Strange comment.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:260: // Subclasses of this class can be e.g.
connected tcp sockets
Please end comment with .

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:261: template<typename SI>
Maybe change SI to DI.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:265: : SI(fd), port_(0), tokens_(16), mask_(0)
{}
Pull 16 into a constant.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:333: template<typename SI>
Maybe change SI to DI.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:488: // Update token count.
The comment above says that we are not counting tokens in this case.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:504: bool was_ready = pentry->IsReady();
Assert tokens >= + before and <= kTokenCount after update.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:148: } else {
Use UpdateEpollInstance here. Maybe also on Android.

[kustermann, 2015-02-09 08:50:05]

New patchsets have been uploaded after l-g-t-m from
wibling@google.com,sgjesse@google.com

[kustermann, 2015-02-09 08:50:22]

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandler.h
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:197: if (head_ != NULL && head_->next_ != NULL)
{
On 2015/02/06 14:09:28, wibling wrote:
> NIT: you could assert head_->next != NULL inside the if and not check.

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:246: // Returns the bit-wise OR of events t
On 2015/02/06 14:42:17, Søren Gjesse wrote:
> Strange comment.

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:260: // Subclasses of this class can be e.g.
connected tcp sockets
On 2015/02/06 14:42:17, Søren Gjesse wrote:
> Please end comment with .

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:261: template<typename SI>
On 2015/02/06 14:42:18, Søren Gjesse wrote:
> Maybe change SI to DI.

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:265: : SI(fd), port_(0), tokens_(16), mask_(0)
{}
On 2015/02/06 14:42:17, Søren Gjesse wrote:
> Pull 16 into a constant.

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:333: template<typename SI>
On 2015/02/06 14:42:17, Søren Gjesse wrote:
> Maybe change SI to DI.

Done.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:488: // Update token count.
On 2015/02/06 14:42:18, Søren Gjesse wrote:
> The comment above says that we are not counting tokens in this case.

It says:

// Will post `data` to all known Dart_Ports. It will also decrease the token
count by 1.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:504: bool was_ready = pentry->IsReady();
On 2015/02/06 14:42:18, Søren Gjesse wrote:
> Assert tokens >= + before and <= kTokenCount after update.

Done. - Though it is still not clear if we can assume that the token count
cannot become negative.

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:148: } else {
On 2015/02/06 14:42:18, Søren Gjesse wrote:
> Use UpdateEpollInstance here. Maybe also on Android.

That would be an infinite recursive call, this function is UpdateEpollInstance!

We could add an optimization to selectively modify the epoll fd and try not to
remove+add it, but I think this is not a very common thing to go from
  kReadEvent -> kWriteEvent
  kWriteEvent -> kReadEvent
  kReadEvent|kWriteEvent -> kReadEvent
  kReadEvent|kWriteEvent -> kWriteEvent
  ...

[kustermann, 2015-02-09 08:50:54]

Committed patchset #2 (id:20001) manually as 43585 (presubmit successful).

[Søren Gjesse, 2015-02-09 09:09:21]

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/905733002/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_linux.cc:148: } else {
On 2015/02/09 08:50:22, kustermann wrote:
> On 2015/02/06 14:42:18, Søren Gjesse wrote:
> > Use UpdateEpollInstance here. Maybe also on Android.
> 
> That would be an infinite recursive call, this function is
UpdateEpollInstance!
> 
> We could add an optimization to selectively modify the epoll fd and try not to
> remove+add it, but I think this is not a very common thing to go from
>   kReadEvent -> kWriteEvent
>   kWriteEvent -> kReadEvent
>   kReadEvent|kWriteEvent -> kReadEvent
>   kReadEvent|kWriteEvent -> kWriteEvent
>   ...

It probably isn't. We should still consider using EPOLL_CTL_MOD (I thought we
already had a method for that).