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] Change metadata pointer type to ExternalData #12433

Closed
wants to merge 1 commit into from

Conversation

ragmani
Copy link
Contributor

@ragmani ragmani commented Jan 10, 2024

This commit changes metadata pointer type to ExternalData.

ONE-DCO-1.0-Signed-off-by: ragmani ragmani0216@gmail.com

This commit changes metadata pointer type to ExternalData.

ONE-DCO-1.0-Signed-off-by: ragmani <ragmani0216@gmail.com>
@ragmani ragmani added the PR/NO MERGE Please don't merge. I'm still working on this :) label Jan 10, 2024
@ragmani ragmani requested a review from zetwhite January 10, 2024 05:01
@zetwhite
Copy link
Contributor

Thanks for trying 😄

I think we can use ExternalData for now and change it to use ir::Data (include CachedData) later when we need it.
I put my detailed opinion here - #12408 (comment).

@ragmani
Copy link
Contributor Author

ragmani commented Jan 10, 2024

I have a question regardless to this PR.
Is model going to have metadata as only external data(i.g. ir::MMapedData) in the future?

Originally posted by @ragmani in #12408 (comment)

@ragmani
Copy link
Contributor Author

ragmani commented Jan 10, 2024

Let me clarify the word first, to make sure that we have the same understanding.

  • ir::CachedData : holds copy of original data
  • ir::ExternalData : points original data, we have to make sure that the original data is not lost.

Originally posted by @zetwhite in #12408 (comment)

@ragmani
Copy link
Contributor Author

ragmani commented Jan 10, 2024

Is model going to have metadata as only external data(i.g. ir::MMapedData) in the future?

@ragmani
For now, only TrainingInfo is passed through Model's Metadata.
Since TrainingInfo is extracted when loaded time, we could use ExternalData.
If Metadata is used(extracted) at another time, I'm not sure that the original buffer (mmaped data) is not damaged.

So, About your question, For now, Yes only external data is used in metadata.
About the future, I'm not 100% sure.

Originally posted by @zetwhite in #12408 (comment)

@ragmani
Copy link
Contributor Author

ragmani commented Jan 10, 2024

  • ir::ExternalData : points original data, we have to make sure that the original data is not lost.

@zetwhite
Can we make sure if original data(external data) is not lost?

@ragmani
Copy link
Contributor Author

ragmani commented Jan 10, 2024

As talked offline with @zetwhite,

  1. We confirmed that mmaped original data(external data) is not lost unless we call munmap(). So metadata cannot be danging pointer in the current code.
  2. @zetwhite is going to resolve the issue of having metadata information twice in memory.(i.g. TrainingInfo and mmaped metadata for TrainingInfo)

For those reasons, I close this PR. Sorry for interrupting you due to my misunderstanding.

@ragmani ragmani closed this Jan 10, 2024
@zetwhite
Copy link
Contributor

For those reasons, I close this PR. Sorry for interrupting you due to my misunderstanding.

No problem at all.
I appreciate your interest and by this chance, I could rethink about overall direction:)

@ragmani ragmani deleted the onert/change_external_data branch July 3, 2024 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/NO MERGE Please don't merge. I'm still working on this :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants