Skip to content

Conversation

@yufenglee
Copy link
Member

With build_essential option, it runs 0.2ms faster for agi-encoder model (total cost 1.6ms ).

@yufenglee yufenglee requested a review from a team as a code owner December 22, 2018 01:42
@jywu-msft
Copy link
Member

jywu-msft commented Dec 22, 2018

Is this PR really needed? Isn't there another PR #197
and previous PR #166
which addresses perf overhead?
or are you finding that those are still not enough?
is this change coordinated with the planned changes mentioned in those PR's?
if it's possible to address the perf overhead without having build option, that seems better to me.

@yufenglee
Copy link
Member Author

@jywu-msft we do need this PR because PR #197 and previous PR #166 doesn't solve the problem completely. There is still overhead for profiling. I make a change without build option.

@yufenglee yufenglee requested review from a team and snnn and removed request for snnn January 1, 2019 02:51
Copy link
Member

@jywu-msft jywu-msft left a comment

Choose a reason for hiding this comment

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

thanks for the updates.

auto graph_viewer = session_state.GetGraphViewer();
TimePoint sync_time_begin;
TimePoint kernel_begin_time;
bool f_profiler_enabled = session_state.Profiler().FEnabled();
Copy link
Member

Choose a reason for hiding this comment

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

bool profiler_enabled = session_state.Profiler().Enabled() reads better?

Copy link
Member Author

Choose a reason for hiding this comment

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

same with the function name, f means flag here.

@yufenglee yufenglee merged commit e274651 into master Jan 1, 2019
@raymondxyang raymondxyang deleted the roli/agiencoder branch January 10, 2019 20:47
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