mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kellen sunderland <kellen.sunderl...@gmail.com>
Subject Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Date Thu, 04 Oct 2018 08:05:36 GMT
I agree that active C++ developers should be the ones making these
choices.  The downside to that is that many of these people are already
pretty busy.  To make the best use possible of their time it would probably
make sense to create a concise doc with proposed style changes and ETAs
rather than focusing on a single change (modernized range loops).  We've
got the infrastructure in place now to support any decisions made, and I
think it's in the best interest of the project as it will (1) unify coding
style to help readability and (2) make the lifes easier for reviewers which
are a critical resource on this project.

I've update my PR such that it still modernizes all existing loops, but it
will now only issue a warning in the case that someone commits an older or
slower version of a loop.  Of course as we've mentioned a few times this
only applies to cases where loops can be drop-in replaces with range
loops.  I.e. no loops indexes are actively used, etc.

Would you be alright with merging this change with warnings instead of
errors for the time being Chris?

On Wed, Oct 3, 2018 at 7:20 PM Pedro Larroy <pedro.larroy.lists@gmail.com>
wrote:

> +1
>
> @Chris: do you have data on the performance difference? As far as I know
> there's a "rewrite rule" like the one between lambdas and C++ functors, so
> performance should be very well defined, but maybe there's something that
> you are point out that we are missing.
>
> Having a consistent and concise code base is beneficial, I think what
> Kellen is advocating is to use range loops whenever possible, not
> prescribing its usage on every case, if you have to iterate backwards there
> are other ways such as backwards iterator or others.
>
> On Fri, Sep 28, 2018 at 6:54 AM Chris Olivier <cjolivier01@apache.org>
> wrote:
>
> > -1
> >
> > Range loops aren’t always the most performant way. In addition, sometimes
> > you want the index. Or maybe you want to iterate backwards, or not start
> > from the first, etc. Maybe you want the iterator because you remove it
> from
> > the list at the bottom of the loop.... Seems like a rule for the sake of
> > having a rule.
> >
> > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland <
> > kellen.sunderland@gmail.com> wrote:
> >
> > > Hello MXNet devs,
> > >
> > > I'd like to discuss uniformly adopting C++11 range loops in the MXNet
> > > project.  The benefits I see are:
> > >
> > > *  Improved C++ readability (examples below).
> > > *  Consistency with other languages.  The range-loops are quite similar
> > to
> > > loops almost all other programming languages.  Given we're a project
> that
> > > supports many languages this language consistency could be positive for
> > our
> > > community.
> > > * Consistency within the same project.  Currently different authors
> have
> > > different loops styles which hurts codebase readability.
> > > *  Best available performance.  There are often multiple ways to write
> > > loops in C++ with subtle differences in performance and memory usage
> > > between loop methods.  Using range-loops ensures we get the best
> possible
> > > perf using an intuitive loop pattern.
> > > *  Slightly lower chance for bugs / OOB accesses when dealing with
> > indexing
> > > in an array for example.
> > >
> > > If we decide to enable this uniformly throughout the project we can
> > enable
> > > this policy with a simple clang-tidy configuration change.  There would
> > be
> > > no need for reviewers to have to manually provide feedback when someone
> > > uses an older C++ loops style.
> > >
> > > -Kellen
> > >
> > > Reference PR:  https://github.com/apache/incubator-mxnet/pull/12356/
> > > Previous clang-tidy discussion on the list:
> > >
> > >
> >
> https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E
> > >
> > > -------------------------
> > > Examples:
> > > for (auto axis_iter = param.axis.begin() ; axis_iter!=
> param.axis.end();
> > > ++axis_iter) {
> > >     CHECK_LT(*axis_iter, static_cast<int>(ishape.ndim()));
> > >     stride_[reverse_index] = ishape[*axis_iter];
> > >     ...
> > > -->
> > > for (int axis : param.axis) {
> > >     CHECK_LT(axis, static_cast<int>(ishape.ndim()));
> > >     stride_[reverse_index] = ishape[axis];
> > >     ...
> > > --------------------------
> > > for (size_t i = 0; i < in_array.size(); i++) {
> > >     auto &nd = in_array[i];
> > >     pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype());
> > > }
> > > -->
> > > for (auto & nd : in_array) {
> > >     pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true, nd.dtype());
> > > }
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message