From dev-return-4260-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Fri Sep 28 16:42:47 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 6509B180675 for ; Fri, 28 Sep 2018 16:42:46 +0200 (CEST) Received: (qmail 75895 invoked by uid 500); 28 Sep 2018 14:42:45 -0000 Mailing-List: contact dev-help@mxnet.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mxnet.incubator.apache.org Delivered-To: mailing list dev@mxnet.incubator.apache.org Received: (qmail 75828 invoked by uid 99); 28 Sep 2018 14:42:44 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 28 Sep 2018 14:42:44 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 723FC18040D for ; Fri, 28 Sep 2018 14:42:44 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.888 X-Spam-Level: * X-Spam-Status: No, score=1.888 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_PASS=-0.001, T_DKIMWL_WL_MED=-0.01] autolearn=disabled Authentication-Results: spamd3-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id pSNq_jm4HrE4 for ; Fri, 28 Sep 2018 14:42:42 +0000 (UTC) Received: from mail-yb1-f181.google.com (mail-yb1-f181.google.com [209.85.219.181]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id 490945F385 for ; Fri, 28 Sep 2018 14:42:42 +0000 (UTC) Received: by mail-yb1-f181.google.com with SMTP id w80-v6so2719863ybe.10 for ; Fri, 28 Sep 2018 07:42:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jG6NZdZA+UEE8qSt7U3QvnsPODeawFqrM8xYkbERDQg=; b=cSzHRqjiBff3qq6kiskQh9aXLFCHNe9LlaNPBFsM16/zmpPL6+b3syJYMMuEwSBLYA pI1kU3XYzcQK0oqoCIICQ4cEE0UKqKlfvah+LYZSJxuMmt+LEAhSFXB19p1C0hMO6W+i 31BJ0Bp4Apm6MDZcJyobDEQJsmS2l2oVNG4dSjbKGLzmHC5U3xzHdXIFD40KTVIs4NOi bqdaD/3J7i9/Sp+M9CwizDa1RQkZvYONWn+Bb1ORCz+FJdu4QnEZZaa5N1IxLBrxfeD/ y2HCRIsUBXzWXIa59blm0RpS5jeIWu/bWtxrv6flX+qnFH02Ot358cDzFKIDJWVbCEL5 sbNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jG6NZdZA+UEE8qSt7U3QvnsPODeawFqrM8xYkbERDQg=; b=Ed+jB5hAYjiI3DWotbOL3EDk8CqI251jY4wYGbiWY9SIuPQiBHqIU2wwycJ/FtKGsu ZNN6Y0ybiJMP+hA3mlKKE3xzB0AQNckfrxD6q3Sey2J8CtwNnM7UotQqSU6z2blwann8 6Lv2HOjhqKRRJZQ0z5Xq0W1WqlkIt/dI8f4VBfdHk9A86bkeerK9yFeevpO4D4F2+2Ii ZVc/di+MS+OJ8Wprk4Od6LErO80k/QCzp6zSOrvBKNskPS8R9s0jESRjKfxYElFEJXtH OjNLakkZjhAdPoHRw2+XyGcUh6FBOPxzhb3A5HP1sHeLgQYgfmpf49ct8ndv+sGguG1+ vSuw== X-Gm-Message-State: ABuFfoifu8iBUObCx0Ewhb6bhQCgXik73qPfILk2ZCUJqPbNUn/Xrkss CGT0NO61Ddn8fBD3/12sPCcwFt2PKfzRygOrk89iGIVpurY= X-Google-Smtp-Source: ACcGV62mSvwPgg5F22StyKoT4RrxSciX+hkrq/3HLwRtSHF69kEJSHW+e9xsjtlfw+4Mrfoots55BVMl+vS6cLP03Gs= X-Received: by 2002:a25:b78a:: with SMTP id n10-v6mr8620485ybh.415.1538145755601; Fri, 28 Sep 2018 07:42:35 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: kellen sunderland Date: Fri, 28 Sep 2018 16:42:25 +0200 Message-ID: Subject: Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project To: dev@mxnet.incubator.apache.org Cc: dev@mxnet.apache.org Content-Type: multipart/alternative; boundary="0000000000006027fa0576ef773d" --0000000000006027fa0576ef773d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable "Range loops aren=E2=80=99t 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 wrote: > -1 > > Range loops aren=E2=80=99t always the most performant way. In addition, s= ometimes > 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 fr= om > 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 th= at > > supports many languages this language consistency could be positive for > our > > community. > > * Consistency within the same project. Currently different authors hav= e > > 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 possib= le > > 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/b0ae5a9df5dfe0d9074cb2ebe432264db4fa= 2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E > > > > ------------------------- > > Examples: > > for (auto axis_iter =3D param.axis.begin() ; axis_iter!=3D param.axis.e= nd(); > > ++axis_iter) { > > CHECK_LT(*axis_iter, static_cast(ishape.ndim())); > > stride_[reverse_index] =3D ishape[*axis_iter]; > > ... > > --> > > for (int axis : param.axis) { > > CHECK_LT(axis, static_cast(ishape.ndim())); > > stride_[reverse_index] =3D ishape[axis]; > > ... > > -------------------------- > > for (size_t i =3D 0; i < in_array.size(); i++) { > > auto &nd =3D 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()); > > } > > > --0000000000006027fa0576ef773d--