Basic PR on Cost Model #35774
Basic PR on Cost Model #35774
Conversation
|
Thanks for your contribution! |
| EnableProfiler(profiler_state); | ||
| executor.Run(main_program, &scope, /*block_id = */ 0); | ||
|
|
||
| std::vector<std::vector<Event>>* time_events = |
There was a problem hiding this comment.
I think the best idea is using unique_ptr or shared_ptr rather than raw pointer.
There was a problem hiding this comment.
Fine to me, change done.
| } | ||
| ++event_index; | ||
| } | ||
| if (start_profiler_idx != 0 && stop_profiler_idx != 0) { |
There was a problem hiding this comment.
start_profiler_idx != 0? Why can not main_thread_events[0].name() == "_start_profiler_", I think it is possible.
There was a problem hiding this comment.
There is a little background, profiler would have some initialize events which aren't "start_profiler", but to support the start_profiler_idx == 0 case, I changed the related code. I compare it with -1;
| ProgramDesc program = CreateTestProgram(); | ||
| ProgramDesc empty_program; | ||
| CostData cost_data = | ||
| cost_model.ProfileMeasure(program, empty_program, "cpu", {"time"}); |
There was a problem hiding this comment.
I didn't find "gpu" version test, should add it for coverage CI?
There was a problem hiding this comment.
You can see it on python test ^_^
| } | ||
| int count = 0; | ||
| while (count <= 1000) { | ||
| ++count; |
There was a problem hiding this comment.
Does this useless loop necessary?
There was a problem hiding this comment.
Necessary to occupy test time
| ++event_index; | ||
| } | ||
| if (start_profiler_idx != -1 && stop_profiler_idx != -1) { | ||
| double cpu_time_ms = main_thread_events[start_profiler_idx].CpuElapsedMs( |
There was a problem hiding this comment.
Add PADDLE_ENFORCE_GE(start_profiler_idx, 0) and PADDLE_ENFORCE_GT(stop_profiler_idx, start_profiler_idx) here?
There was a problem hiding this comment.
Not good now, because we are not sure about profiler
| // here. | ||
| } | ||
|
|
||
| double CostData::GetOpTimeMs(int op_id) const { return op_time_ms_.at(op_id); } |
There was a problem hiding this comment.
I think it's better to use size_t replace int because the global_block.OpSize() return size_t.
There was a problem hiding this comment.
A little background, we would also support graph in the future, then there will be int node id. We are not sure whether there is negative graph id
Add basic Cost Model, it uses executor to run program and profile it to get op time. This is an early basic version, we will add more functions in the future.
Add basic Cost Model, it uses executor to run program and profile it to get op time. This is an early basic version, we will add more functions in the future.
PR types
New features
PR changes
APIs
Describe
Add basic Cost Model, it uses executor to run program and profile it to get op time.
This is an early basic version, we will add more functions in the future.