From dev-return-4367-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Wed Oct 3 19:20:50 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 1E54E18065B for ; Wed, 3 Oct 2018 19:20:49 +0200 (CEST) Received: (qmail 78736 invoked by uid 500); 3 Oct 2018 17:20:49 -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 78723 invoked by uid 99); 3 Oct 2018 17:20:48 -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; Wed, 03 Oct 2018 17:20:48 +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 0D53A1809E7 for ; Wed, 3 Oct 2018 17:20:48 +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 NfrIfeH0-2yw for ; Wed, 3 Oct 2018 17:20:46 +0000 (UTC) Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com [209.85.208.173]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id DC17B5F18F for ; Wed, 3 Oct 2018 17:20:45 +0000 (UTC) Received: by mail-lj1-f173.google.com with SMTP id v6-v6so5838432ljc.11 for ; Wed, 03 Oct 2018 10:20:45 -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; bh=T54706PiJc53gY/w+lH0E0x9UzaiP+fDski2fblMikA=; b=pLJwfy3vPSJo1K+nRwz1uKTCn/P7kq89tlKJQdMHkvJW9Ys2bUoS/Wc78Egzcb5+fo jmfT1OhiVELbPBqdjtcUCFssmpm58WJx+z9URKE8IhOgc//Rinp5/ZEKRm/qgH4359YM 0y1nPmQUaKl2KWmpOkwcb/zH+6ZdlRSVvvvQ9XBw7gu8GrpCoNmrl3J+ZbWiDSbW2VDw Bqqh0sPXamYCCjr/de2GhXR3bLva0AbcgB+ANWnblUB1ARNUkIJeoVd/TEibMc7Z4l6J YGvfvuyF7wYuCycjHyJ70BEHRmfiPsxoE3f6Z5yEB1VgFnsJ54GaycM7+fgyqjYR69EG OXdw== 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; bh=T54706PiJc53gY/w+lH0E0x9UzaiP+fDski2fblMikA=; b=CILHJwkjsySVygKSXKCLftpel0LIuxV27btbqRZquVqG5wxsmNvqc9Kam3yotdZxnD gp9HAn73hHerO+6KatCFc4nsC2864vZz7dER27InjcTFtDYYCyrrH9PChMYc+/uMXwP3 JDKYSu85BfB0ljhvZCThx1gELEuN5zk+qoqwacq1gYKgiU7sgsIR/gIkFTuI8wWLQiyg BtRLfqm2hxTq7KNk1oxvIUNzrF2w/2c0w9bSKxuhtScZX6tefPXANmOTTvURsUOkRcXU EogSt3fL3rZ5TdryQBhlQDMjFD9gU4iCqaXC0S1R5T88oSHGn+pYASyoafZpMXEBq36A rs9Q== X-Gm-Message-State: ABuFfoiN4vdTcIwq63YNL5g5NL7QM430xw4iaq2HQLwA8WdX2w0yJMRJ IrheuNZdliqg4pdjpbIK2v/Cev9pgjqZ29TSQgc8OQ== X-Google-Smtp-Source: ACcGV61G8dMN7OQN9LNzyhXGL7YSO7KAxfApKqX6B7xz2sx0lWdvuMLPJLAQK9vdogdT+gCXvQbuWeq9Q9ulEwz7FIM= X-Received: by 2002:a2e:b04b:: with SMTP id d11-v6mr1736240ljl.93.1538587238891; Wed, 03 Oct 2018 10:20:38 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Pedro Larroy Date: Wed, 3 Oct 2018 10:20:27 -0700 Message-ID: Subject: Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project To: dev@mxnet.incubator.apache.org Content-Type: multipart/alternative; boundary="000000000000d485af0577564192" --000000000000d485af0577564192 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable +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 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()); > > } > > > --000000000000d485af0577564192--