Request: allow importing Boost's/Asio's coroutine.hpp

[Christian Biesinger]

Hi all!

In https://codereview.chromium.org/2242483003/ it was suggested I email the style guide group for guidance, so here I am.

For layoutng (https://groups.google.com/a/chromium.org/d/msg/layout-dev/A-9v6MyCTRY/Ti0HogzjAwAJ) we would like to have interruptible layout, so as to avoid layout-induced jank.

The approach we'd like to use is coroutines like from asio:

http://think-async.com/Asio/asio-1.10.6/doc/asio/reference/coroutine.html

Their coroutines header is basically the same as a boost one, and boost is largely not allowed by the style guide:

https://google.github.io/styleguide/cppguide.html#Boost

We would like to ask for an exception for this header, for the specific use case of using it in LayoutNG.

Thanks,

-Christian

[Jeffrey Yasskin]

I've bcc'ed Asio's author and the author of the C++ Coroutines
proposal as an FYI. Follow along at
https://groups.google.com/a/chromium.org/d/topic/cxx/b9pEd1MX5WU/discussion.

The coroutines this allows are also the same shape as proposed for
C++Next (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0057r5.pdf),
and implemented in MSVC:
https://blogs.msdn.microsoft.com/vcblog/2015/11/30/coroutines-in-visual-studio-2015-update-1/.
Gor's also working on a clang and LLVM implementation:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102337.html.

I haven't checked whether there are subtle differences between the C++
proposal and the ASIO implementation.

Jeffrey

[Brett Wilson]

I think there are several different issues:

1. Are coroutines a good concept?

2. Can coroutines be used successfully across Chrome without making a mess or being confusing?

3. Can the coroutine shim being proposed be successfully used across Chrome?

My answers:

1. Yes.

2. Hopefully but I feel like there needs to be a fair bit of guidance and education. Historically Chrome has leaned on the Google-wide C++ style team to provide this, and that doesn't seem to existy yet.

3. I have no idea. I would like to see some examples. The example in the header and it doesn't even seem to compile (it uses "yield" and "fork" that aren't defined). Since this is a new concept, a style of programming I'm not experienced with, and there's not the guidance yet in #2, I'm having a hard time understanding these examples even assuming I mentally substitute ASIO_CORO_YIELD. I think it would be helpful to see a more real example of how this header in particular would be used to implement something we care about.

Brett

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANh-dXnGgkvmuzTv9tCsFtRfccJa-TT9vWYQrOBP8aTkcPpYxw%40mail.gmail.com.

[Jeremy Roman]

(Disclaimer: I don't have experience using this library in practice, but I've now read and roughly understand the implementation.)

I can see coroutines being a good feature with language support, but I'm not thrilled about importing this library into Chromium, especially using it in core/ code which can already be pretty intimidating to read.

I am generally not a fan of excessive amounts of macro magic, and this is pretty magical (and has quite a few caveats for which our toolchain won't give good diagnostic messages, nor do I expect debugging such issues to be easy). For instance, absolutely no non-trivial variable names must be defined across yield statements, because their constructor and/or destructor site may be skipped. It's very clever, but IMHO excessively so (at least measured against the Google code style), and due to the limitations of the language it's a leaky abstraction.

I already know of reviewers who are requesting that usage of lambda be limited, and that's a feature with considerably simpler semantics and considerably better tooling, to the best of my knowledge. For what it's worth, this library is still in the disallowed section of boost internally.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

[Ian Kilpatrick]

I've written up a basic overview of what we'd like to achieve; written in pseudocode-JS

See here:

https://gist.github.com/bfgeek/3c372942b942652bb841028ce2f7e849

(obviously this would written in C++ for blink, but we have generator-yield functions in JS and not C++ yet :).

Inside of Blink we'd like to make a lot of the subsystems (style/layout/etc) be able to time-slice.

As a concrete example: run as much layout as possible for 3ms.

This is for things like asyncAppend[1], and speculative layout.

For other subsystems (e.g. style) there isn't a lot of internal state to keep around to perform time-slicing. It is pretty reasonable to perform style inheritance in a "ticking" manner.

Layout on the other hand has a lot of internal state (just look at how many and what type of layout passes flexbox performs) which is kept around between ticks.

Ideally we'd just use C++XX generator functions which would fit this perfectly. We don't live in that world yet and would like to something to achieve this now.

We've previously tried to do some more complicated layouts without a generator-yield like affordance, and it ends up to be a large mental burden.

We'd like to use this specifically for the new LayoutNG code which lives in core/layout/ng. This wouldn't be exposed to any old or existing blink code.

Thanks,

[1] https://github.com/drufball/async-append

[Brett Wilson]

I'm pretty skeptical of this shim. I agree with what Jeremy said about compiler support and macro magic. I also can't evaluate my opinion of the shim more specifically from JavaScript pseudocode.  I don't understand at all the object lifetime semantics of this (based on Jeremy's alarming concerns around destructors across yield, it seems like maybe this just doesn't work). While this shim may be great for simple demos, it seems to me that the realities of the complex layout engine and some years of evolution and bugfixes will force this into a very messy and hard-to-understand place we'll come to regret.

In the end, I think that the language isn't there yet to express things the way you want. I agree with the current Google-wide policy that this should remain disallowed.

Brett

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJL3UpRbtjwinuvsPZSsQH1wgWTR7i_vp_szYbM67dm8v2tuXQ%40mail.gmail.com.

[Christian Biesinger]

So here's actual code that compiles. I could also port the Source/core/layout/ng/ng_block_flow_layout_algorithm.cc to this if that would be helpful. But here's an example:

https://gist.github.com/cbiesinger/e9c10fcfb0791919594783dab251f8ee

Note that if you do something that would skip the initialization, you get a compile error, so it is not such a subtle error.

Also, I don't think it's fair to summarize the Google style guide as disallowing coroutines. It contains a blanket prohibition against most boost modules, but not a specific one against coroutines.

Hope that helps!

-Christian

[Brett Wilson]

On Tue, Aug 16, 2016 at 5:38 PM, Christian Biesinger <cbies...@chromium.org> wrote:

So here's actual code that compiles. I could also port the Source/core/layout/ng/ng_block_flow_layout_algorithm.cc to this if that would be helpful. But here's an example:

https://gist.github.com/cbiesinger/e9c10fcfb0791919594783dab251f8ee

Note that if you do something that would skip the initialization, you get a compile error, so it is not such a subtle error.

Also, I don't think it's fair to summarize the Google style guide as disallowing coroutines. It contains a blanket prohibition against most boost modules, but not a specific one against coroutines.

Right, but we should generally be following the same rules. We can make exceptions, but my read is that neither cxx nor eng-review is very excited about this.

My main objection is the macros that try to emulate the constructs of a very high level language with built-in support for coroutines. This node is not understandable by random people on the team, and I suspect is not debuggable even by you.

I feel like you have a very specific need and want to do layout in a very specific way. You've discovered that coroutines are a good way to express that. But to accomplish this goal I suspect it's not required that we add macro magic to support this very general paradigm, nor is it even required that we add general non-macro support for coroutine-style programming to our codebase (that you pointed out in an internal thread also exists).

I think the best thing for you to do is figure out the exact minimal thing you need and spend some time massaging some classes to express the exact minimal think you want very clearly. You have chunks of layout work, I think you can make something with clear names, that programmers can step into in a debugger, and that this specific pattern can be documented so people can understand it when they see it. A surgically-specific approach to how you want layout to work will be much easier to explain and maintain for our large team over a long period of time than the overly general and not-easily-explained concept of coroutines.

Brett

[Jeremy Roman]

On Tue, Aug 16, 2016 at 8:38 PM, Christian Biesinger <cbies...@chromium.org> wrote:

So here's actual code that compiles. I could also port the Source/core/layout/ng/ng_block_flow_layout_algorithm.cc to this if that would be helpful. But here's an example:

https://gist.github.com/cbiesinger/e9c10fcfb0791919594783dab251f8ee

This code doesn't compile for me; at the very least everything inside <> seems to have been stripped.

 

Note that if you do something that would skip the initialization, you get a compile error, so it is not such a subtle error.

Ah, good that our compilers do catch it if you skip initialization, though of course if you skip a non-initializing declaration g++ won't complain, and clang++ requires only catches it due to -Wsometimes-uninitialized -Werror (which I think is covered by our flags). And the compiler will warn before you skip a non-trivial destructor. That does make me feel somewhat better.
https://gist.github.com/jeremyroman/7152901a577722c1331934b0d535d709

Of course, if you do lift the local variable and initialize it outside of the "reenter", you get no warnings whatsoever. Similarly if you use any of the parameter variables.

 

Also, I don't think it's fair to summarize the Google style guide as disallowing coroutines. It contains a blanket prohibition against most boost modules, but not a specific one against coroutines.

My reading of the rules in the style guide and the internal go/whynotboost suggests that it would be disallowed if proposed; it's a very new pattern that makes code using it harder to correctly understand for someone unfamiliar with the library in question (I've now spent a few hours understanding it, and only now do I think I might be able to write correct code with it), and you have to write code in a very particular way to be correct (even if the compiler will warn about some of the worse ones). I'm worried about the amount of time an average Chromium engineer would have to spend getting to grips with.

Additionally, you've even run into a subtle bug in the example you provided, albeit ones that full warnings would have caught. You used the "statement" form of yield, which does not return a value. Instead, all of your yields actually return true, because by failing to use "yield return", you just transferred control to the end of the reenter block (where your other return statement executes). You intended to write "ASIO_CORO_YIELD return false;", which actually sets the result of the coroutine invocation.

And the warning you get even with all warnings on is this (with clang):

error: expression result unused [-Werror,-Wunused-value]

      ASIO_CORO_YIELD false;

                      ^~~~~

But even that warning wouldn't be produced if you were returning something more complicated which the compiler couldn't trivially prove was unused, like the result of a function call. In that case, clang++ and g++ both produce no warnings with "-Wall -Werror".

I'm sorry, but I'd still rather we not adopt this library in Chromium.

[Jeffrey Yasskin]

On Wed, Aug 17, 2016 at 8:23 AM, Jeremy Roman <jbr...@chromium.org> wrote:

On Tue, Aug 16, 2016 at 8:38 PM, Christian Biesinger <cbies...@chromium.org> wrote:

So here's actual code that compiles. I could also port the Source/core/layout/ng/ng_block_flow_layout_algorithm.cc to this if that would be helpful. But here's an example:

https://gist.github.com/cbiesinger/e9c10fcfb0791919594783dab251f8ee

This code doesn't compile for me; at the very least everything inside <> seems to have been stripped.

 

Note that if you do something that would skip the initialization, you get a compile error, so it is not such a subtle error.

Ah, good that our compilers do catch it if you skip initialization, though of course if you skip a non-initializing declaration g++ won't complain, and clang++ requires only catches it due to -Wsometimes-uninitialized -Werror (which I think is covered by our flags). And the compiler will warn before you skip a non-trivial destructor. That does make me feel somewhat better.
https://gist.github.com/jeremyroman/7152901a577722c1331934b0d535d709

Of course, if you do lift the local variable and initialize it outside of the "reenter", you get no warnings whatsoever. Similarly if you use any of the parameter variables.

Of course Christian could write a clang plugin to catch that sort of thing.

 

Also, I don't think it's fair to summarize the Google style guide as disallowing coroutines. It contains a blanket prohibition against most boost modules, but not a specific one against coroutines.

My reading of the rules in the style guide and the internal go/whynotboost suggests that it would be disallowed if proposed; it's a very new pattern that makes code using it harder to correctly understand for someone unfamiliar with the library in question (I've now spent a few hours understanding it, and only now do I think I might be able to write correct code with it), and you have to write code in a very particular way to be correct (even if the compiler will warn about some of the worse ones). I'm worried about the amount of time an average Chromium engineer would have to spend getting to grips with.

Additionally, you've even run into a subtle bug in the example you provided, albeit ones that full warnings would have caught. You used the "statement" form of yield, which does not return a value. Instead, all of your yields actually return true, because by failing to use "yield return", you just transferred control to the end of the reenter block (where your other return statement executes). You intended to write "ASIO_CORO_YIELD return false;", which actually sets the result of the coroutine invocation.

And the warning you get even with all warnings on is this (with clang):

error: expression result unused [-Werror,-Wunused-value]

      ASIO_CORO_YIELD false;

                      ^~~~~

Is this one diagnosable with a clang plugin, or does it look the same as valid code? Christian, I think it's up to you to find this sort of pitfall before Jeremy does, and show that you can catch it.

Jeffrey

[Brett Wilson]

I don't want to get too bogged down in particular details here. Of course, Christian is welcome to do research if he wants. But my read is that a clang plugins to catch big mistakes here isn't sufficient to make this seem like a good idea, and I don't want him to feel like he should spend time on this to win people over.

I continue to think you should design something specifically to implement the need you have for your particular layout algorithm.

Brett

[Jeffrey Yasskin]

On Tue, Aug 16, 2016 at 10:09 PM, Brett Wilson <bre...@chromium.org> wrote:

I feel like you have a very specific need and want to do layout in a very specific way. You've discovered that coroutines are a good way to express that. But to accomplish this goal I suspect it's not required that we add macro magic to support this very general paradigm, nor is it even required that we add general non-macro support for coroutine-style programming to our codebase (that you pointed out in an internal thread also exists).

I think the best thing for you to do is figure out the exact minimal thing you need and spend some time massaging some classes to express the exact minimal think you want very clearly. You have chunks of layout work, I think you can make something with clear names, that programmers can step into in a debugger, and that this specific pattern can be documented so people can understand it when they see it. A surgically-specific approach to how you want layout to work will be much easier to explain and maintain for our large team over a long period of time than the overly general and not-easily-explained concept of coroutines.

This strikes me as similar to saying that Javascript doesn't need async/await, that callbacks are just fine. I don't think that's a common position for Javascript, and it shouldn't be a common position for C++.

Jeffrey

[Antoine Labour]

Well, javascript is a different language, it's hard to compare. C++ doesn't have garbage collection or weak typing either.

I think I see value in coroutines if they're a first class citizen in the language, supported by the compiler and tools (debugger, etc.). I have more doubts if they are implemented using macro goop - one more DSL to learn :(

Have we considered simply using a thread that is in lock-step with the main thread as an alternative?

Antoine

Jeffrey

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANh-dXkSJN3k_-hSE-CbXtTL17y3mwsXymssEGzkGmnofAdgNw%40mail.gmail.com.

[Jeffrey Yasskin]

On Wed, Aug 17, 2016 at 1:50 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Aug 17, 2016 at 1:22 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Tue, Aug 16, 2016 at 10:09 PM, Brett Wilson <bre...@chromium.org> wrote:

I feel like you have a very specific need and want to do layout in a very specific way. You've discovered that coroutines are a good way to express that. But to accomplish this goal I suspect it's not required that we add macro magic to support this very general paradigm, nor is it even required that we add general non-macro support for coroutine-style programming to our codebase (that you pointed out in an internal thread also exists).

I think the best thing for you to do is figure out the exact minimal thing you need and spend some time massaging some classes to express the exact minimal think you want very clearly. You have chunks of layout work, I think you can make something with clear names, that programmers can step into in a debugger, and that this specific pattern can be documented so people can understand it when they see it. A surgically-specific approach to how you want layout to work will be much easier to explain and maintain for our large team over a long period of time than the overly general and not-easily-explained concept of coroutines.

This strikes me as similar to saying that Javascript doesn't need async/await, that callbacks are just fine. I don't think that's a common position for Javascript, and it shouldn't be a common position for C++.

Well, javascript is a different language, it's hard to compare. C++ doesn't have garbage collection or weak typing either.

I think I see value in coroutines if they're a first class citizen in the language, supported by the compiler and tools (debugger, etc.). I have more doubts if they are implemented using macro goop - one more DSL to learn :(

I totally agree that the cost of doing coroutines with macros could be too high. I'd love to see the outcome of Christian's investigation of how they fall down before deciding that firmly.

[Christian Biesinger]

In light of the opposition to the proposal, I want to withdraw it. We will go ahead with a regular state machine and see how that works out for us.

-Christian

[Peter Kasting]

On Thu, Aug 18, 2016 at 11:33 AM, Christian Biesinger <cbies...@chromium.org> wrote:

In light of the opposition to the proposal, I want to withdraw it. We will go ahead with a regular state machine and see how that works out for us.

FWIW, I think coroutines conceptually can be a cool tool (if a rather special-purpose one), and if C++17 or 20 get better first-class support for them, I hope a lot of the opposition here would go away, as it seems largely directed at the implementation rather than the concept.

But sadly the time when we could use such features in the codebase is at least six years from now or something so that doesn't help :(

PK