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 Fri, 28 Sep 2018 14:42:25 GMT
"Range loops aren’t always the most performant way" Do you have an example
where there's a perf difference?

"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."

I should have been more clear about this point.  If you're using the index
in the loop, doing reverse iteration, or not iterating from start-to-end
this inspection is smart enough to realize it and will not suggest
optimizing that type of loop.  The loops that would be changes are _only_
the loops which are detected as equivalent to range-loops.  Examples can be
found here:
https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
or you can look at what's been changed in the ref PR.  I've initially set
our confidence level at 'reasonable' but we could also set to 'safe' which
would further reduce the number of loops the check would apply to.

-Kellen

On Fri, Sep 28, 2018 at 3:54 PM 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