Skip to content

Conversation

@lum1n0us
Copy link
Collaborator

No description provided.

@lum1n0us lum1n0us force-pushed the log_rss branch 2 times, most recently from 09ce776 to 4b23a9f Compare November 24, 2022 06:23
* @return 0 if success, -1 otherwise
*/
int
os_dumps_proc_mem_info(char *out, unsigned int size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

[minor] could we use size_t -like type for the size variable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think we could probably make the API more verbose - having char * type as a return value doesn't tell implementation and the user how the data should be structured (so on windows we might have different format, on linux there will be a different format).
I'd suggest using a well-defined structure that's being returned here so:

  1. implementers know exactly what sort of data needs to be returned
  2. users of that method know what to expect
  3. we can have a consistent formatting defined in a common layer, rather than have different formatting for each system
  4. If we ever want to emit metrics from WAMR or generate statistics, we can't rely on unstructured text.

This is just a suggestion - if returning an unstructured blob from this function is really what we want, that's fine, just wanted to raise the concern and make sure we're making a conscious decision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am thinking that os_dumps_proc_mem_info should return some kind of memory usage reports of the current process. The developer should not be worried about how the memory data is structured or how different every platform is. It doesn't limit the output format and the output items.

Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh wenyongh merged commit eaedcec into bytecodealliance:main Nov 25, 2022
NingW101 pushed a commit to NingW101/wasm-micro-runtime that referenced this pull request Dec 1, 2022
…odealliance#1734)

Only support Posix platforms currently, read memory consumption info from
file "/proc/self/status".
wenyongh pushed a commit that referenced this pull request Dec 6, 2022
Only support Posix platforms currently, read memory consumption info from
file "/proc/self/status".
@lum1n0us lum1n0us deleted the log_rss branch December 7, 2022 07:24
wenyongh pushed a commit that referenced this pull request Dec 19, 2022
Only support Posix platforms currently, read memory consumption info from
file "/proc/self/status".
vickiegpt pushed a commit to vickiegpt/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…odealliance#1734)

Only support Posix platforms currently, read memory consumption info from
file "/proc/self/status".
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.

3 participants