Force vector call operator to be inlined.

[galv]

I noticed in a real world program that about 5% of time is spent
in this method, so this seems like a reasonable thing to force,
given the small nature of this function.

Note that I am not sure whether this will force icc to inline the
member functions. I doubt it.

See discussion at #2406

[galv]

Not tested on windows, or with intel's compiler. But put this up just to show how small the change is.

[jtrmal]

So did you test before and after? I'd be interested (just to satiate my curiosity) in seeing what is the cost of the function call itself (compared to the "body" of the function). y.

…

On Tue, May 8, 2018 at 12:02 PM Daniel Galvez ***@***.***> wrote: Not tested on windows, or with intel's compiler. But put this up just to show how small the change is. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKisX9vUb14fdFQouU3dZuReKQr4NLasks5twcGTgaJpZM4T26hk> .

[galv]

Hi Yenda! It's a little bit hard to profile, since I know of know profiling tools that can count time spent in an inlined function (since at that point, you no longer know that it was a function). I could quite reasonably make a vector-speed-test.cc to really see what's going on, though.

…

On Tue, May 8, 2018 at 9:06 AM, jtrmal ***@***.***> wrote: So did you test before and after? I'd be interested (just to satiate my curiosity) in seeing what is the cost of the function call itself (compared to the "body" of the function). y. On Tue, May 8, 2018 at 12:02 PM Daniel Galvez ***@***.***> wrote: > Not tested on windows, or with intel's compiler. But put this up just to > show how small the change is. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ AKisX9vUb14fdFQouU3dZuReKQr4NLasks5twcGTgaJpZM4T26hk> > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEi_UMs5PlZlAxeBAAFCugSP24gpHauUks5twcKggaJpZM4T26hk> .

-- Daniel Galvez http://danielgalvez.me https://github.com/galv

[jtrmal]

Hm, I guess you are right, profiling an inlined function doesn't make any sense. y. On Tue, May 8, 2018 at 12:11 PM Daniel Galvez <notifications@github.com> wrote:

…

Hi Yenda! It's a little bit hard to profile, since I know of know profiling tools that can count time spent in an inlined function (since at that point, you no longer know that it was a function). I could quite reasonably make a vector-speed-test.cc to really see what's going on, though. On Tue, May 8, 2018 at 9:06 AM, jtrmal ***@***.***> wrote: > So did you test before and after? I'd be interested (just to satiate my > curiosity) in seeing what is the cost of the function call itself (compared > to the "body" of the function). > y. > > On Tue, May 8, 2018 at 12:02 PM Daniel Galvez ***@***.***> > wrote: > > > Not tested on windows, or with intel's compiler. But put this up just to > > show how small the change is. > > > > — > > You are receiving this because you are subscribed to this thread. > > Reply to this email directly, view it on GitHub > > <#2407 (comment)>, > or mute > > the thread > > <https://github.com/notifications/unsubscribe-auth/ > AKisX9vUb14fdFQouU3dZuReKQr4NLasks5twcGTgaJpZM4T26hk> > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AEi_UMs5PlZlAxeBAAFCugSP24gpHauUks5twcKggaJpZM4T26hk > > . > -- Daniel Galvez http://danielgalvez.me https://github.com/galv — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKisX5l2ER_mNv6BFqND5jRK3OTpUXPwks5twcOVgaJpZM4T26hk> .

[galv]

Results (see the latest commit for the speed test's contents):

Nearly 2x the speed, from what I can tell.

Float, not inlined:

VectorBase::operator(),float,1024,5.00679e-06,seconds
VectorBase::operator(),float,2048,1.00136e-05,seconds
VectorBase::operator(),float,4096,2.00272e-05,seconds
VectorBase::operator(),float,8192,3.91006e-05,seconds

Float, inlined:

VectorBase::operator(),float,1024,3.09944e-06,seconds
VectorBase::operator(),float,2048,5.96046e-06,seconds
VectorBase::operator(),float,4096,1.19209e-05,seconds
VectorBase::operator(),float,8192,2.38419e-05,seconds

Double, not inlined:

VectorBase::operator(),double,1024,5.00679e-06,seconds
VectorBase::operator(),double,2048,1.00136e-05,seconds
VectorBase::operator(),double,4096,2.00272e-05,seconds
VectorBase::operator(),double,8192,4.00543e-05,seconds

Double, inlined:

VectorBase::operator(),double,1024,2.86102e-06,seconds
VectorBase::operator(),double,2048,5.96046e-06,seconds
VectorBase::operator(),double,4096,1.19209e-05,seconds
VectorBase::operator(),double,8192,2.40803e-05,seconds

[danpovey]

Interesting. Incidentally, I sometimes optimize out the use of () if it occurs in inner loops, by using pointers-- because the operator () has a bounds check in it unless you call with -NDEBUG, and I doubt the compilers are smart enough to fully optimize that out. But it still makes sense to optimize it if we can. @galv, can you at least test on mac, since I believe they use clang? (Although they name it g++, so I don't know if that means they emulate g++ features to some extent)... Dan

…

On Tue, May 8, 2018 at 12:39 PM, Daniel Galvez ***@***.***> wrote: Results (see the latest commit for the speed test's contents): Nearly 2x the speed, from what I can tell. Float, not inlined: VectorBase::operator(),float,1024,5.00679e-06,seconds VectorBase::operator(),float,2048,1.00136e-05,seconds VectorBase::operator(),float,4096,2.00272e-05,seconds VectorBase::operator(),float,8192,3.91006e-05,seconds Float, inlined: VectorBase::operator(),float,1024,3.09944e-06,seconds VectorBase::operator(),float,2048,5.96046e-06,seconds VectorBase::operator(),float,4096,1.19209e-05,seconds VectorBase::operator(),float,8192,2.38419e-05,seconds Double, not inlined: VectorBase::operator(),double,1024,5.00679e-06,seconds VectorBase::operator(),double,2048,1.00136e-05,seconds VectorBase::operator(),double,4096,2.00272e-05,seconds VectorBase::operator(),double,8192,4.00543e-05,seconds Double, inlined: VectorBase::operator(),double,1024,2.86102e-06,seconds VectorBase::operator(),double,2048,5.96046e-06,seconds VectorBase::operator(),double,4096,1.19209e-05,seconds VectorBase::operator(),double,8192,2.40803e-05,seconds — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu8vPZRfI2e1kKCqrgbdJk7VedX3Kks5twco2gaJpZM4T26hk> .

[galv]

Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely wait until tonight, though.

::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by default no bounds checking (regardless of the NDEBUG macros)? The fact that the implementation is otherwise just adding to a pointer and dereferencing made me surprised that this function is not inlined by default.

[danpovey]

You're right, it won't check unless you define -DKALDI_PARANOID.

…

On Tue, May 8, 2018 at 1:59 PM, Daniel Galvez ***@***.***> wrote: Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely wait until tonight, though. ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by default no bounds checking (regardless of the NDEBUG macros)? The fact that the implementation is otherwise just adding to a pointer and dereferencing made me surprised that this function is not inlined by default. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu2t5HFdqrqWvI8036sRbN22ncC0_ks5twd0PgaJpZM4T26hk> .

[jtrmal]

IIRC and AFAIK, the inlining can be disabled/enabled by a switch of gcc compiler (I just checked and its -finline-functions) I think for low -O degrees (or -Os) the switch can by by default disabled (i.e. no inlining unless -O1 or -O2). But I'm mostly speculating... y.

…

On Tue, May 8, 2018 at 1:59 PM Daniel Galvez ***@***.***> wrote: Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely wait until tonight, though. ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by default no bounds checking (regardless of the NDEBUG macros)? The fact that the implementation is otherwise just adding to a pointer and dereferencing made me surprised that this function is not inlined by default. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> .

[jtrmal]

see this: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html seems like inlining is enabled in -Os, -O2 and -O3 y.

…

On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: IIRC and AFAIK, the inlining can be disabled/enabled by a switch of gcc compiler (I just checked and its -finline-functions) I think for low -O degrees (or -Os) the switch can by by default disabled (i.e. no inlining unless -O1 or -O2). But I'm mostly speculating... y. On Tue, May 8, 2018 at 1:59 PM Daniel Galvez ***@***.***> wrote: > Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely > wait until tonight, though. > > ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by > default no bounds checking (regardless of the NDEBUG macros)? The fact that > the implementation is otherwise just adding to a pointer and dereferencing > made me surprised that this function is not inlined by default. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> > . >

[jtrmal]

actually, I think that the attribute is superfluous and compiling the libs with higher -O or just adding the switch (-finline-functions) for deployment (which seems Daniel's use-case) would make more sense. y.

…

On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: see this: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html seems like inlining is enabled in -Os, -O2 and -O3 y. On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > IIRC and AFAIK, the inlining can be disabled/enabled by a switch of > gcc compiler (I just checked and its -finline-functions) > I think for low -O degrees (or -Os) the switch can by by default disabled > (i.e. no inlining unless -O1 or -O2). > But I'm mostly speculating... > y. > > On Tue, May 8, 2018 at 1:59 PM Daniel Galvez ***@***.***> > wrote: > >> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely >> wait until tonight, though. >> >> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by >> default no bounds checking (regardless of the NDEBUG macros)? The fact that >> the implementation is otherwise just adding to a pointer and dereferencing >> made me surprised that this function is not inlined by default. >> >> — >> You are receiving this because you commented. >> Reply to this email directly, view it on GitHub >> <#2407 (comment)>, >> or mute the thread >> <https://github.com/notifications/unsubscribe-auth/AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> >> . >> >

[galv]

Looks like -finline-functions gets set only at optimization level -O3. And something else I didn't realization is that Kaldi is built by default without optimizations. Whoops! I'll check if -O2, which enables -finline-small-functions, will actually inline operator().

…

On Tue, May 8, 2018 at 11:14 AM, jtrmal ***@***.***> wrote: actually, I think that the attribute is superfluous and compiling the libs with higher -O or just adding the switch (-finline-functions) for deployment (which seems Daniel's use-case) would make more sense. y. On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: > see this: > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > seems like inlining is enabled in -Os, -O2 and -O3 > y. > > On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > >> IIRC and AFAIK, the inlining can be disabled/enabled by a switch of >> gcc compiler (I just checked and its -finline-functions) >> I think for low -O degrees (or -Os) the switch can by by default disabled >> (i.e. no inlining unless -O1 or -O2). >> But I'm mostly speculating... >> y. >> >> On Tue, May 8, 2018 at 1:59 PM Daniel Galvez ***@***.***> >> wrote: >> >>> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely >>> wait until tonight, though. >>> >>> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is by >>> default no bounds checking (regardless of the NDEBUG macros)? The fact that >>> the implementation is otherwise just adding to a pointer and dereferencing >>> made me surprised that this function is not inlined by default. >>> >>> — >>> You are receiving this because you commented. >>> Reply to this email directly, view it on GitHub >>> <#2407 (comment)>, >>> or mute the thread >>> <https://github.com/notifications/unsubscribe-auth/ AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> >>> . >>> >> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEi_UL49GV-bkXAnmzvq-lKx8qqrwCRXks5tweCbgaJpZM4T26hk> .

-- Daniel Galvez http://danielgalvez.me https://github.com/galv

[jtrmal]

I think you will be pleasantly surprised how much faster some algorithms will get. Y.

…

On Tue, May 8, 2018, 15:42 Daniel Galvez ***@***.***> wrote: Looks like -finline-functions gets set only at optimization level -O3. And something else I didn't realization is that Kaldi is built by default without optimizations. Whoops! I'll check if -O2, which enables -finline-small-functions, will actually inline operator(). On Tue, May 8, 2018 at 11:14 AM, jtrmal ***@***.***> wrote: > actually, I think that the attribute is superfluous and compiling the libs > with higher -O or just adding the switch (-finline-functions) for > deployment (which seems Daniel's use-case) would make more sense. > y. > > > On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: > > > see this: > > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > > seems like inlining is enabled in -Os, -O2 and -O3 > > y. > > > > On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > > > >> IIRC and AFAIK, the inlining can be disabled/enabled by a switch of > >> gcc compiler (I just checked and its -finline-functions) > >> I think for low -O degrees (or -Os) the switch can by by default > disabled > >> (i.e. no inlining unless -O1 or -O2). > >> But I'm mostly speculating... > >> y. > >> > >> On Tue, May 8, 2018 at 1:59 PM Daniel Galvez < ***@***.***> > >> wrote: > >> > >>> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll likely > >>> wait until tonight, though. > >>> > >>> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is > by > >>> default no bounds checking (regardless of the NDEBUG macros)? The fact > that > >>> the implementation is otherwise just adding to a pointer and > dereferencing > >>> made me surprised that this function is not inlined by default. > >>> > >>> — > >>> You are receiving this because you commented. > >>> Reply to this email directly, view it on GitHub > >>> <#2407 (comment) >, > >>> or mute the thread > >>> <https://github.com/notifications/unsubscribe-auth/ > AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> > >>> . > >>> > >> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AEi_UL49GV-bkXAnmzvq-lKx8qqrwCRXks5tweCbgaJpZM4T26hk > > . > -- Daniel Galvez http://danielgalvez.me https://github.com/galv — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKisX7KmlYoWodQY0Iyjusi5b_94IZ0Uks5twfUwgaJpZM4T26hk> .

[danpovey]

I think the default if you run 'configure' now is to have optimization. If you configured a long time ago it might have been built with -O0.. or maybe you added that while debugging something.

…

On Tue, May 8, 2018 at 3:50 PM, jtrmal ***@***.***> wrote: I think you will be pleasantly surprised how much faster some algorithms will get. Y. On Tue, May 8, 2018, 15:42 Daniel Galvez ***@***.***> wrote: > Looks like -finline-functions gets set only at optimization level -O3. And > something else I didn't realization is that Kaldi is built by default > without optimizations. Whoops! I'll check if -O2, which enables > -finline-small-functions, will actually inline operator(). > > On Tue, May 8, 2018 at 11:14 AM, jtrmal ***@***.***> wrote: > > > actually, I think that the attribute is superfluous and compiling the > libs > > with higher -O or just adding the switch (-finline-functions) for > > deployment (which seems Daniel's use-case) would make more sense. > > y. > > > > > > On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: > > > > > see this: > > > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > > > seems like inlining is enabled in -Os, -O2 and -O3 > > > y. > > > > > > On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > > > > > >> IIRC and AFAIK, the inlining can be disabled/enabled by a switch of > > >> gcc compiler (I just checked and its -finline-functions) > > >> I think for low -O degrees (or -Os) the switch can by by default > > disabled > > >> (i.e. no inlining unless -O1 or -O2). > > >> But I'm mostly speculating... > > >> y. > > >> > > >> On Tue, May 8, 2018 at 1:59 PM Daniel Galvez < > ***@***.***> > > >> wrote: > > >> > > >>> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll > likely > > >>> wait until tonight, though. > > >>> > > >>> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there is > > by > > >>> default no bounds checking (regardless of the NDEBUG macros)? The > fact > > that > > >>> the implementation is otherwise just adding to a pointer and > > dereferencing > > >>> made me surprised that this function is not inlined by default. > > >>> > > >>> — > > >>> You are receiving this because you commented. > > >>> Reply to this email directly, view it on GitHub > > >>> <#2407 (comment)- 387489379 > >, > > >>> or mute the thread > > >>> <https://github.com/notifications/unsubscribe-auth/ > > AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> > > >>> . > > >>> > > >> > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > <#2407 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/AEi_UL49GV-bkXAnmzvq- lKx8qqrwCRXks5tweCbgaJpZM4T26hk > > > > . > > > > > > -- > Daniel Galvez > http://danielgalvez.me > https://github.com/galv > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ AKisX7KmlYoWodQY0Iyjusi5b_94IZ0Uks5twfUwgaJpZM4T26hk> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuyw19ZB_0W1IK1eBnW7lsWlKxbtCks5twfcjgaJpZM4T26hk> .

[galv]

From what I can tell, by default there are no optimizations when you just run ./configure by itself. Anyway, I just checked, and with EXTRA_CXXFLAGS="-O2" make -j will inline the call operator for VectorBase. So that's good. It may be worthwhile to consider turning on optimizations by default when you did ./configure (Maybe -O1 or -O2?). The default level in gcc (and clang) is -O0 (no optimizations). Since this had to do with a misunderstanding on my part, I will probably close this pull request in a few hours. On Tue, May 8, 2018 at 1:42 PM, Daniel Povey <notifications@github.com> wrote:

…

I think the default if you run 'configure' now is to have optimization. If you configured a long time ago it might have been built with -O0.. or maybe you added that while debugging something. On Tue, May 8, 2018 at 3:50 PM, jtrmal ***@***.***> wrote: > I think you will be pleasantly surprised how much faster some algorithms > will get. > Y. > > > On Tue, May 8, 2018, 15:42 Daniel Galvez ***@***.***> wrote: > > > Looks like -finline-functions gets set only at optimization level -O3. > And > > something else I didn't realization is that Kaldi is built by default > > without optimizations. Whoops! I'll check if -O2, which enables > > -finline-small-functions, will actually inline operator(). > > > > On Tue, May 8, 2018 at 11:14 AM, jtrmal ***@***.***> > wrote: > > > > > actually, I think that the attribute is superfluous and compiling the > > libs > > > with higher -O or just adding the switch (-finline-functions) for > > > deployment (which seems Daniel's use-case) would make more sense. > > > y. > > > > > > > > > On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: > > > > > > > see this: > > > > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > > > > seems like inlining is enabled in -Os, -O2 and -O3 > > > > y. > > > > > > > > On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > > > > > > > >> IIRC and AFAIK, the inlining can be disabled/enabled by a switch of > > > >> gcc compiler (I just checked and its -finline-functions) > > > >> I think for low -O degrees (or -Os) the switch can by by default > > > disabled > > > >> (i.e. no inlining unless -O1 or -O2). > > > >> But I'm mostly speculating... > > > >> y. > > > >> > > > >> On Tue, May 8, 2018 at 1:59 PM Daniel Galvez < > > ***@***.***> > > > >> wrote: > > > >> > > > >>> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll > > likely > > > >>> wait until tonight, though. > > > >>> > > > >>> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that there > is > > > by > > > >>> default no bounds checking (regardless of the NDEBUG macros)? The > > fact > > > that > > > >>> the implementation is otherwise just adding to a pointer and > > > dereferencing > > > >>> made me surprised that this function is not inlined by default. > > > >>> > > > >>> — > > > >>> You are receiving this because you commented. > > > >>> Reply to this email directly, view it on GitHub > > > >>> <#2407 (comment)- > 387489379 > > >, > > > >>> or mute the thread > > > >>> <https://github.com/notifications/unsubscribe-auth/ > > > AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> > > > >>> . > > > >>> > > > >> > > > > > > — > > > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > > > <#2407 (comment) >, > > or mute > > > the thread > > > < > > https://github.com/notifications/unsubscribe- auth/AEi_UL49GV-bkXAnmzvq- > lKx8qqrwCRXks5tweCbgaJpZM4T26hk > > > > > > . > > > > > > > > > > > -- > > Daniel Galvez > > http://danielgalvez.me > > https://github.com/galv > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#2407 (comment)>, > or mute > > the thread > > <https://github.com/notifications/unsubscribe-auth/ > AKisX7KmlYoWodQY0Iyjusi5b_94IZ0Uks5twfUwgaJpZM4T26hk> > > . > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ADJVuyw19ZB_ 0W1IK1eBnW7lsWlKxbtCks5twfcjgaJpZM4T26hk> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEi_UDeBuyzw5Vl-UrT0DAHXuNSeOWBfks5twgMpgaJpZM4T26hk> .

-- Daniel Galvez http://danielgalvez.me https://github.com/galv

[danpovey]

You are right. I had thought that the default optimization level in gcc was -O2, but it looks like I was wrong. I think we should probably have at least -O1 be the default (I'm concerned about going to high by default, since compilation does take a while). Perhaps you could work on changing some of the files in makefiles/ instead? Dan On Tue, May 8, 2018 at 5:58 PM, Daniel Galvez <notifications@github.com> wrote:

…

From what I can tell, by default there are no optimizations when you just run ./configure by itself. Anyway, I just checked, and with EXTRA_CXXFLAGS="-O2" make -j will inline the call operator for VectorBase. So that's good. It may be worthwhile to consider turning on optimizations by default when you did ./configure (Maybe -O1 or -O2?). The default level in gcc (and clang) is -O0 (no optimizations). Since this had to do with a misunderstanding on my part, I will probably close this pull request in a few hours. On Tue, May 8, 2018 at 1:42 PM, Daniel Povey ***@***.***> wrote: > I think the default if you run 'configure' now is to have optimization. If > you configured a long time ago it might have been built with -O0.. or maybe > you added that while debugging something. > > > > On Tue, May 8, 2018 at 3:50 PM, jtrmal ***@***.***> wrote: > > > I think you will be pleasantly surprised how much faster some algorithms > > will get. > > Y. > > > > > > On Tue, May 8, 2018, 15:42 Daniel Galvez ***@***.***> > wrote: > > > > > Looks like -finline-functions gets set only at optimization level -O3. > > And > > > something else I didn't realization is that Kaldi is built by default > > > without optimizations. Whoops! I'll check if -O2, which enables > > > -finline-small-functions, will actually inline operator(). > > > > > > On Tue, May 8, 2018 at 11:14 AM, jtrmal ***@***.***> > > wrote: > > > > > > > actually, I think that the attribute is superfluous and compiling the > > > libs > > > > with higher -O or just adding the switch (-finline-functions) for > > > > deployment (which seems Daniel's use-case) would make more sense. > > > > y. > > > > > > > > > > > > On Tue, May 8, 2018 at 2:10 PM Jan Trmal ***@***.***> wrote: > > > > > > > > > see this: > > > > > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > > > > > seems like inlining is enabled in -Os, -O2 and -O3 > > > > > y. > > > > > > > > > > On Tue, May 8, 2018 at 2:05 PM Jan Trmal ***@***.***> wrote: > > > > > > > > > >> IIRC and AFAIK, the inlining can be disabled/enabled by a switch > of > > > > >> gcc compiler (I just checked and its -finline-functions) > > > > >> I think for low -O degrees (or -Os) the switch can by by default > > > > disabled > > > > >> (i.e. no inlining unless -O1 or -O2). > > > > >> But I'm mostly speculating... > > > > >> y. > > > > >> > > > > >> On Tue, May 8, 2018 at 1:59 PM Daniel Galvez < > > > ***@***.***> > > > > >> wrote: > > > > >> > > > > >>> Yes, I can do it on Mac, where g++ masquerades as clang++. It'll > > > likely > > > > >>> wait until tonight, though. > > > > >>> > > > > >>> ::operator() uses KALDI_PARANOID_ASSERT(), so I thought that > there > > is > > > > by > > > > >>> default no bounds checking (regardless of the NDEBUG macros)? The > > > fact > > > > that > > > > >>> the implementation is otherwise just adding to a pointer and > > > > dereferencing > > > > >>> made me surprised that this function is not inlined by default. > > > > >>> > > > > >>> — > > > > >>> You are receiving this because you commented. > > > > >>> Reply to this email directly, view it on GitHub > > > > >>> <#2407 (comment)- > > 387489379 > > > >, > > > > >>> or mute the thread > > > > >>> <https://github.com/notifications/unsubscribe-auth/ > > > > AKisX5uvdXLG2JjI5t4DQszgR8bVakMdks5twd0TgaJpZM4T26hk> > > > > >>> . > > > > >>> > > > > >> > > > > > > > > — > > > > You are receiving this because you were mentioned. > > > > Reply to this email directly, view it on GitHub > > > > <#2407 (comment)- 387493787 > >, > > > or mute > > > > the thread > > > > < > > > https://github.com/notifications/unsubscribe- > auth/AEi_UL49GV-bkXAnmzvq- > > lKx8qqrwCRXks5tweCbgaJpZM4T26hk > > > > > > > > . > > > > > > > > > > > > > > > > -- > > > Daniel Galvez > > > http://danielgalvez.me > > > https://github.com/galv > > > > > > — > > > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > > > <#2407 (comment) >, > > or mute > > > the thread > > > <https://github.com/notifications/unsubscribe-auth/ > > AKisX7KmlYoWodQY0Iyjusi5b_94IZ0Uks5twfUwgaJpZM4T26hk> > > > . > > > > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#2407 (comment)>, > or mute > > the thread > > <https://github.com/notifications/unsubscribe-auth/ADJVuyw19ZB_ > 0W1IK1eBnW7lsWlKxbtCks5twfcjgaJpZM4T26hk> > > > . > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#2407 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AEi_UDeBuyzw5Vl- UrT0DAHXuNSeOWBfks5twgMpgaJpZM4T26hk> > . > -- Daniel Galvez http://danielgalvez.me https://github.com/galv — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVu6CoSWJHjFDfkwXXEztu-9SC2GToks5twhULgaJpZM4T26hk> .

[galv]

What you propose sounds reasonable. I am sure I can do that at some point. I'll just need to make sure I put -O1 in the right location so that that setting can be overridden by the user specifying EXTRA_CXXFLAGS (e.g., if EXTRA_CXXFLAGS="-O2", then we should not compile with -O1), which involves studying the gcc man page for a bit.

[galv]

Interestingly, it turns out that enabling -O2 by default (in an internal kaldi repo) has created a lot interesting new warnings (I guess because inlining allows for more whole-program analysis). For example:

kaldi-io.cc: In member function ‘virtual bool kaldi::OffsetFileInputImpl::Open(const string&, bool)’:
kaldi-io.cc:586:27: warning: ‘offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       return (is_.tellg() == std::streampos(offset));
                           ^
kaldi-io.cc:624:14: note: ‘offset’ was declared here
       size_t offset;
              ^
kaldi-io.cc:586:27: warning: ‘offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       return (is_.tellg() == std::streampos(offset));
                           ^
kaldi-io.cc:608:14: note: ‘offset’ was declared here
       size_t offset;
              ^
parse-options.cc: In member function ‘int32 kaldi::ParseOptions::ToInt(const string&)’:
parse-options.cc:599:10: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   return ret;
          ^
parse-options.cc: In member function ‘uint32 kaldi::ParseOptions::ToUint(const string&)’:
parse-options.cc:606:10: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   return ret;
          ^

So it won't be a small PR, most likely.

I'll get around to it later.

[galv]

Superseded by #2411