Time measurement support on build. (#319) #330

ed728 · 2019-06-19T11:31:30Z

Motivation and Context

Debug options should not be default, so I made the time measurement feature a togglable option for the blueoil.sh script. For running make inside of the project, you would have to manually append the CXXFLAGS, so make lm_fpga CXXFLAG="-DFUNC_TIME_MEASUREMENT".

This will be followed by proper user documentation for #319.

Description

-Extra optional option added to blueoil.sh.
-Removed timing as a default for lm_fpga target.
-CXXFLAGS now specified via make argument, rather than environment.
-Timing is now only done when specified, not always by default. Debug options should not be default...

How has this been tested?

Jenkins.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature / Optimization (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.

ed728 · 2019-06-19T11:33:09Z

I am not too confident about what the automated Jenkins tests cover, so someone please poke me in case it is not sufficient. I did run make test-dlk locally, and no errors popped in the beginning, but I am not sure whether it uses these scripts or not...

Also, as far as I am aware we have no tests in CI which rely on the time measurement feature, so nothing should break.

ed728 · 2019-06-19T11:33:27Z

@iizukak Any suggestions for reviewers?

ed728 · 2019-06-19T11:34:15Z

Also @n-nez, since I expect you guys would be ever so slightly affected by the Makefile change.

primenumber · 2019-06-19T11:37:26Z

@ed728 make test-dlk uses CMake instead of raw Makefile...

iizukak · 2019-06-20T00:10:47Z

run blueoil test

iizukak

@ed728 looks good except I commented issue.

iizukak · 2019-06-20T00:16:52Z

blueoil/blueoil_convert.py

+    "--timing",
+    help="enable operation time measurements",
+    type=int,
+    default=0,


The default value shoud be False or None not integer.

iizukak · 2019-06-20T00:18:25Z

@ed728 tca_support is already merged. Please change base branch to master .

ed728 · 2019-06-20T00:37:49Z

@primenumber Yeah, Jenkins does cover the normal use case though.

n-nez · 2019-06-20T02:24:49Z

-Timing is now only done when specified, not always by default. Debug options should not be the default...

Actually, the only uses of lm_fpga are

testing
time measurement

So keeping time measurement enabled by default for the target sounds sensible for me actually. Benefits of switching it off, on the other hand, is not clear. 🤔

ed728 · 2019-06-20T04:54:48Z

Hmmm, I mean this stems from #319 which wants documentation for time measurements. And when looking through our documentation, for people wanting to run on FPGA it suggests running blueoil.sh which eventually invokes make lm_fpga... So any customer or external person who would like to use blueoil with an FPGA would go through that build as I see it.

Also, even for testing and time measuring, being able to disable internal debugs for "external"/"real" time measurements from OS is useful I think (i.e. timing how ./lm_fpga.elf ... takes as a whole with not other internal debug options enabled).

Oooor, I am missing something important about how customers are supposed to use this.

Ref.: https://docs.blue-oil.org/usage/convert.html

n-nez · 2019-06-20T05:08:26Z

If "extenal" time measurements imply measurement of the execution time of whole binary itself, they do not make much sense to me, as there are lots of "preparation steps" like loading npy files and Network::init(), so measurements will be rather misleading.

Customers aren't supposed to use lm_fpga.elf at all, it's purely diagnostic/testing tool, to check that dlk conversion works as expected.

Customers supposed to use lib_fpga.so in their applications, for which TIME_MEASUREMENT is disabled, so they could do whatever kind of measurements which suits them if required.

n-nez · 2019-06-20T05:14:58Z

As for #319 documentation should say how to

obtain npys from training checkpoint
convert pb file
do make lm_fpga
run lm_fpga.elf with the npys and read the report

any mechanism to enable disable measurements in lm_fpga.elf in this case just increases the number of unnecessary work to get the numbers.

ed728 · 2019-06-20T05:39:45Z

It does make sense to measure all loadings and inits for a program that is meant to be used by customers, I think, but that is a different issue.

And I see....but in that case one should be able to enable it for the lm_fpga.so file I think.

Also, the lm_*.elf targets should either all include the measurement flag in the Makefile, or none of them should, I would say.

n-nez · 2019-06-20T06:17:20Z

It does make sense to measure all loadings and inits for a program that is meant to be used by customers

Ok, this is a test program, for developers, customers could run it too if they want, but this is all.

Also, the lm_*.elf targets should either all include the measurement flag in the Makefile, or none of them should, I would say.

All of them should include it, I believe, but so far lm_fpga.elf was the only one target performance of which were relevant to us. I would be very appreciated if you add time measurement flag to all other elf targets we have in this PR, instead of removing it from lm_fpga.elf.

For lm_fpga.so I guess it is doable to enable time measurement for them as well, but the current way we measure time is not suitable for measurements across long runs.

That is it adds a new time interval on each function call into an internal data structure.

If you run inference a few times it will provide you with useful statistics, so you can see how much time the function takes to do work on each layer.
If you run inference million times which supposed to happen when you use lm_fpga.so the structure will grow unlimited and you will run out of memory at the end.
- thus using a normal profiler seems to be a more sensible approach in this case.

ruimashita · 2019-06-20T08:02:18Z

@ed728 Thanks 👍
lm_*.elf are only for test and debug. It is no need flag of enable or disable time measurement of these lm_*.elf, It always should measure time 🤔

iizukak · 2019-06-21T00:21:05Z

#330 (comment)
This @n-nez comment is what I want to do in #319 .
Sorry for confusion.

ed728 · 2019-06-21T05:21:39Z

I will change this to enable it on all lm_*.elf.

On a side note, do we measure the time of things like Network::init() separately?

I still believe it should then be available for lm_*.so as a flag. I mean if it is disabled by default, no one will get an overflow just because of ignorance. And people who do know how to use it, may be able to get some nice data out of it (if we add some reasonable documentation to it).
Also, for this overflow issue, should we make an issue and fix it (eventually)?

n-nez · 2019-06-22T06:03:12Z

On a side note, do we measure the time of things like Network::init() separately?

Yes we do. You will see both init time and run time in the report. 😏

Also, for this overflow issue, should we make an issue and fix it (eventually)?

I believe priority of fixing it is quite low at the moment, but adding an issue definitely doesn't hurt.

iizukak · 2019-07-10T00:52:24Z

@ed728 Can you solve conflict?

ed728 · 2019-07-18T05:42:55Z

@iizukak Yes! Will do that as soon as possible.

iizukak

@ed728
Sorry for review delaying.
Recent, we added TARGETS_X86_AVX target.
Please modify with new target.

Then I'll merge this PR.

ed728 · 2019-08-15T02:05:38Z

@iizukak Fixed, I think. Waiting for tests.

-Extra optional option added to blueoil.sh. -Added timing as a default for lm_* targets . -CXXFLAGS now specified via make argument, rather than environment. -Timing can now optionally be built-in for lib_* targets as well.

ed728 · 2019-09-12T21:19:05Z

@iizukak Any chance this can get approved?

iizukak · 2019-09-13T00:28:17Z

@ed728
Can you answer @ruimashita 's comment?

iizukak · 2019-09-13T00:31:01Z

@ed728
Please check my understand.

You added --timing option for lib_* . And always add TIME_MEASUREMENT option for lm_*. Because lm_* is for debug only and lib_* is not debug only, correct?

CLAassistant · 2020-06-12T06:49:02Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

CLAassistant · 2020-06-12T06:49:16Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ed728 force-pushed the make_edit branch from 84a29be to 35ad416 Compare June 20, 2019 00:10

iizukak reviewed Jun 20, 2019

View reviewed changes

ed728 changed the base branch from tca_support to master June 20, 2019 00:37

ed728 force-pushed the make_edit branch from fe89213 to b7b24b8 Compare June 20, 2019 00:58

ed728 force-pushed the make_edit branch from b7b24b8 to a6a70ec Compare June 20, 2019 04:55

ed728 force-pushed the make_edit branch from a6a70ec to 827125f Compare June 20, 2019 05:54

ruimashita force-pushed the master branch from 6f65af2 to 4456f1c Compare June 26, 2019 07:14

ed728 force-pushed the make_edit branch 3 times, most recently from 73127a9 to b3fba51 Compare July 4, 2019 02:01

ed728 mentioned this pull request Jul 9, 2019

Documentation for time measurement. #358

Merged

5 tasks

hadusam added the CI: auto-run Run CI automatically label Jul 12, 2019

yd8534976 mentioned this pull request Jul 30, 2019

Fix Makefile.tpl #344

Merged

5 tasks

iizukak suggested changes Jul 30, 2019

View reviewed changes

ed728 force-pushed the make_edit branch 2 times, most recently from 1f6c2c3 to 388ff34 Compare August 15, 2019 01:59

ed728 force-pushed the make_edit branch 2 times, most recently from ae893a6 to 428bf0f Compare August 15, 2019 07:40

Time measurement support on build. (blue-oil#319)

84edc14

-Extra optional option added to blueoil.sh. -Added timing as a default for lm_* targets . -CXXFLAGS now specified via make argument, rather than environment. -Timing can now optionally be built-in for lib_* targets as well.

ed728 force-pushed the make_edit branch from 6f10d20 to 84edc14 Compare September 11, 2019 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time measurement support on build. (#319) #330

Time measurement support on build. (#319) #330

ed728 commented Jun 19, 2019

ed728 commented Jun 19, 2019

ed728 commented Jun 19, 2019

ed728 commented Jun 19, 2019

primenumber commented Jun 19, 2019

iizukak commented Jun 20, 2019

iizukak left a comment

iizukak Jun 20, 2019 •

edited

Loading

iizukak commented Jun 20, 2019

ed728 commented Jun 20, 2019

n-nez commented Jun 20, 2019

ed728 commented Jun 20, 2019 •

edited

Loading

n-nez commented Jun 20, 2019 •

edited

Loading

n-nez commented Jun 20, 2019 •

edited

Loading

ed728 commented Jun 20, 2019

n-nez commented Jun 20, 2019

ruimashita commented Jun 20, 2019

iizukak commented Jun 21, 2019

ed728 commented Jun 21, 2019 •

edited

Loading

n-nez commented Jun 22, 2019 •

edited

Loading

iizukak commented Jul 10, 2019

ed728 commented Jul 18, 2019

iizukak left a comment

ed728 commented Aug 15, 2019 •

edited

Loading

ed728 commented Sep 12, 2019

iizukak commented Sep 13, 2019

iizukak commented Sep 13, 2019 •

edited

Loading

CLAassistant commented Jun 12, 2020

CLAassistant commented Jun 12, 2020

Time measurement support on build. (#319) #330

Are you sure you want to change the base?

Time measurement support on build. (#319) #330

Conversation

ed728 commented Jun 19, 2019

Motivation and Context

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

ed728 commented Jun 19, 2019

ed728 commented Jun 19, 2019

ed728 commented Jun 19, 2019

primenumber commented Jun 19, 2019

iizukak commented Jun 20, 2019

iizukak left a comment

Choose a reason for hiding this comment

iizukak Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

iizukak commented Jun 20, 2019

ed728 commented Jun 20, 2019

n-nez commented Jun 20, 2019

ed728 commented Jun 20, 2019 • edited Loading

n-nez commented Jun 20, 2019 • edited Loading

n-nez commented Jun 20, 2019 • edited Loading

ed728 commented Jun 20, 2019

n-nez commented Jun 20, 2019

ruimashita commented Jun 20, 2019

iizukak commented Jun 21, 2019

ed728 commented Jun 21, 2019 • edited Loading

n-nez commented Jun 22, 2019 • edited Loading

iizukak commented Jul 10, 2019

ed728 commented Jul 18, 2019

iizukak left a comment

Choose a reason for hiding this comment

ed728 commented Aug 15, 2019 • edited Loading

ed728 commented Sep 12, 2019

iizukak commented Sep 13, 2019

iizukak commented Sep 13, 2019 • edited Loading

CLAassistant commented Jun 12, 2020

CLAassistant commented Jun 12, 2020

iizukak Jun 20, 2019 •

edited

Loading

ed728 commented Jun 20, 2019 •

edited

Loading

n-nez commented Jun 20, 2019 •

edited

Loading

n-nez commented Jun 20, 2019 •

edited

Loading

ed728 commented Jun 21, 2019 •

edited

Loading

n-nez commented Jun 22, 2019 •

edited

Loading

ed728 commented Aug 15, 2019 •

edited

Loading

iizukak commented Sep 13, 2019 •

edited

Loading