[onert] Introduce metadata to model and loader

[zetwhite]

This PR introduces a metadata field to ir::Model and related loading logic.

draft : #12152
issue : #11692

ONE-DCO-1.0-Signed-off-by: SeungHui Youn sseung.youn@samsung.com

[zetwhite]

Since both tflite and circle have metadata, Let add Metadata and loadMetadata in base_loader.

• tflite_schema.fbs
  

  ONE/runtime/onert/frontend/tflite/tflite_schema.fbs

  Lines 1239 to 1244 in 1162023

  	table Metadata {
  	// A human readable string to uniquely identify a Metadata.
  	name:string;
  	// An index to the buffers table.
  	buffer:uint;
  	}

• circle_schema.fbs (v0.7)
  

  ONE/res/CircleSchema/0.7/circle_schema.fbs

  Lines 1616 to 1621 in 1162023

  	table Metadata {
  	// A human readable string to uniquely identify a Metadata.
  	name:string;
  	// An index to the buffers table.
  	buffer:uint;
  	}

[hseok-oh]

LGTM

[ragmani]

LGTM

[glistening]

I am waiting for is_metadata_exist is renamed.

[zetwhite]

updated method name exists_metadata - 2eddc06
updated get_metadata to return unique_ptr - 2eddc06

[glistening]

@zetwhite It doesn't look intuitive to erase the metadata after get_metadata. Do we need to return unique_ptr?

[zetwhite]

Umm, I've also thought about returning const raw ptr instead of unique_ptr.

I think that returning unique_ptr is much safer.
I'm a little afraid that under situation happened with const raw ptr.

 // load model from file
std::unique_ptr<ir::Model> model = laodModel();
 
// get buffer
const ir::Data* metadata_buffer = model->get_metadata("CIRCLE_TRAINING"); 

// delete a model
// Since the model holds unique_ptr of metadata, all metadata are also deleted.
model.reset(); 

// access already freed memory space (use after free) 
auto base = metadata_buffer->base;

[zetwhite]

Since ir::Model doesn't know how methods can be used, I thought it is better to use unique_ptr for safety.
If you have a better idea about this, plz let me know!

+) If erasing logic with a get_metadata is unintuitive, we could try other naming instead of get_metadata - 🤔 🤔 maybe extract_metadata?

[ragmani]

@zetwhite

+) If erasing metadata afterget_metadata is un-intutive, we could find a bettter naming instead of get_metadata- 🤔 🤔 maybe extract_metadata?

extract_metadata looks better to me.

Since ir::Model doesn't know how methods can be used, I thought it is better to use unique_ptr for safety.
Do you have better idea about this, plz let me know!

I also think unique_ptr is safer. But I'm not familiar with providing only functions for adding/extracting a data type in a class(ir::Model) without providing functions for getting/eliminating the data type(ir::Data).
How about introducing a class for metadata into onert? If so, you can prevent the problem you worried about by adjusting the introduced class (i.g. introducing getting const reference of metadata class into ir::Model and deleting copy constructor of metadata class). Furthermore, you can take advantage of easily adding functions related to metadata.

[glistening]

I don't object to use unique_ptr. unique_ptr is good thing to prevent memory leak. I just don't want to get the user surprised the metadata is removed after get_metadata. If you choose to remove metadata (though I don't like it), it would be better to rename which says clearly the metadata will not be available once you get it. At least for me, extract does not say. I tried to find more appropriate word (release, move, transfer, ...), but these are not clear also.

Once again, I don't oppose this PR if it gets two approvals. It will be merged.

[zetwhite]

How about introducing a class for metadata into onert? If so, you can prevent the problem you worried about by adjusting the introduced class (i.g. introducing getting const reference of metadata class into ir::Model and deleting copy constructor of metadata class). Furthermore, you can take advantage of easily adding functions related to metadata.

@ragmani Thanks for thinking together. I failed to fully understand your suggestion.
If we return a 'const reference of metadata' from get_metadata, Does it extend the lifetime of metadata, even if the model instance is deleted?

Furthermore, you can take advantage of easily adding functions related to metadata.

(not important, but for sharing opinion)
Umm.. I think there are not many functions needed for metadata. Since metadata in a model is just a buffer(not parsed yet), there aren't many common things between metadata.
So, This advantage is not very attractive to me.

[zetwhite]

For now, I'd like to rename get_metadata to extract_metadata and add some comments for ir::Model user.

If I could get 2 approvals, let it proceed.
Or Let me try to find another solution.

(note) I get the word extract from c++17 std.

unlinks the node that contains the element pointed to by position and returns a node handle that owns it.
(https://en.cppreference.com/w/cpp/container/map/extract)

[glistening]

Wow, I didn't know map.extract(). Then, we may replace the body of extract_metadata:

    auto m = _metadatas.find(name);

    if (m == _metadatas.end())
      throw std::out_of_range{"no meatdata named " + name};

    auto data = std::move(m->second);
    _metadatas.erase(m);
    return data;

with:

   // After c++17
   return std::move(_metadatas.extract(name).mapped());

[ragmani]

If we return a 'const reference of metadata' from get_metadata, Does it extend the lifetime of metadata, even if the model instance is deleted?

No, it does not extend lifetime of metadata. If the reference exists and the model instance is deleted, the reference is a dangling reference.

(not important, but for sharing opinion)
Umm.. I think there are not many functions needed for metadata. Since metadata in a model is just a buffer(not parsed yet), there aren't many common things between metadata.
So, This advantage is not very attractive to me.

I see, it was just a suggestion.

[glistening]

_fd must outlive returned std::unique_ptr<ir::Data>. Currently @zetwhite is going to convert ir::Data to TrainingInfo in model loading phase. Thus, it will be safe at this moment.

[zetwhite]

Ah, I also worried about this part.

As I searched, there is no need to keep _fd(file descriptor) open after mmaped.

---

I refered this : https://stackoverflow.com/questions/17490033/do-i-need-to-keep-a-file-open-after-calling-mmap-on-it

• From Posix manpage ref

The mmap() function adds an extra reference to the file associated with the file descriptor fildes which is not removed by a subsequent close() on that file descriptor. This reference is removed when there are no more mappings to the file.

• From Linux manpage ref

On the other hand, closing the file descriptor does not unmap the region.

[glistening]

LGTM 👍

[ragmani]

LGTM