Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[onert] Add MinMaxRecorder Observer #10642

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Conversation

glistening
Copy link
Contributor

It adds MinMaxRecorder as ExecutionObserver.

ONE-DCO-1.0-Signed-off-by: Sanggyu Lee sg5.lee@samsung.com

Related: #10604

@glistening glistening added the PR/NO MERGE Please don't merge. I'm still working on this :) label Apr 14, 2023
Comment on lines 438 to 439
: std::make_unique<exec::MinMaxRecorder>(options.minmax_filepath,
lowered_graph->graph(), backend_contexts);
Copy link
Contributor Author

@glistening glistening Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hseok-oh

It is code from #10610 (comment).

  • It saves reference to backend_contexts, which is going to std::move'd.
    • It would be better to borrow backend_contexts from LinearExecutor after std::move.
  • It assumes LinearExecutor outlives MinMaxRecorder.
    • I will add comment about this assumption.

I will add a commit for this modification.

(+)
I've found backend_contexts is not the only one std::move'd. LoweredGraph works in same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I guessed, previous code gets segmentation fault because it accesses to std::move'd objects. I added fix commit.

It adds MinMaxRecorder as ExecutionObserver.

ONE-DCO-1.0-Signed-off-by: Sanggyu Lee <sg5.lee@samsung.com>
@@ -25,6 +25,7 @@
#include "../exec/ExecTime.h"
#include "../exec/ExecutionObservers.h"
#include "../exec/LinearExecutor.h"
#include "../exec/MinMaxRecorder.h"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android and Tizen (lack of HDF5) cannot use MinMaxRecorder

It does not add MinMaxRecorder for Tizen/Android.
It fixes illegal access to std::move'd objects
@glistening glistening removed the PR/NO MERGE Please don't merge. I'm still working on this :) label Apr 14, 2023
@glistening
Copy link
Contributor Author

$ MINMAX_FILEPATH=iv3.h5 Product/x86_64-linux.debug/out/bin/onert_run \
--load build/install/dataset.h5.0 \
inception_v3_.circle

Model Filename inception_v3_.circle
===================================
MODEL_LOAD   takes 49.872 ms
PREPARE      takes 170.779 ms
EXECUTE      takes 6381.484 ms
- MEAN     :  6381.484 ms
- MAX      :  6381.484 ms
- MIN      :  6381.484 ms
- GEOMEAN  :  6381.484 ms
===================================

$ h5dump iv3.h5 | head -n 20
HDF5 "iv3.h5" {
GROUP "/" {
   GROUP "value" {
      GROUP "0" {
         GROUP "0" {
            GROUP "0" {
               DATASET "0" {
                  DATATYPE  H5T_IEEE_F32BE
                  DATASPACE  SIMPLE { ( 2 ) / ( 2 ) }
                  DATA {
                  (0): 0, 11.1378
                  }
               }
               DATASET "1" {
                  DATATYPE  H5T_IEEE_F32BE
                  DATASPACE  SIMPLE { ( 2 ) / ( 2 ) }
                  DATA {
                  (0): 0, 15.2343
                  }
               }

@hseok-oh hseok-oh merged commit cc25ea0 into Samsung:master Apr 14, 2023
@glistening glistening deleted the let_mmr_observe branch April 14, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants