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

[Tracing] change TraceEvent type from fixed size string to char #3080

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@nickgg
Copy link
Contributor

commented Jun 11, 2019

Summary: TraceEvents have a 1 char type value which we were storing as a string internally. Change to a char to save some memory inside the Runtime.

Documentation: n/a we already export chars through Onnxifi.

Test Plan: unit tests, specifically TraceEventTests.

@jackm321
Copy link
Contributor

left a comment

Looks good!
Just to throw it out there again, why not just use an enum backed by a char? Less things to go wrong.

Show resolved Hide resolved include/glow/ExecutionContext/ExecutionContext.h Outdated
@rdzhabarov
Copy link
Contributor

left a comment

🚢

@nickgg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@jackm321 The thing with an enum is you have to specify all valid options, but we don't support all types in the Trace Event spec. However, it is useful to be able to insert events of a different type sometimes, e.g. as we do in the tracing-compare example to set thread names as Metadata events (type 'M').

@nickgg nickgg force-pushed the nickgg:traceTypeChar branch from e989e60 to 7b040b9 Jun 11, 2019

@facebook-github-bot
Copy link

left a comment

@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.