perf-ninja icon indicating copy to clipboard operation
perf-ninja copied to clipboard

Improve validation in loop_interchange_1

Open dendibakh opened this issue 3 years ago • 6 comments

Comment by @OleksandrKvl :

Current validation is strange in a way that you're using other functions from solution.cpp, not just the main power(). Ideally, you should have separate implementation inside validate.cpp, multiple some matrices with functions from validate.cpp and solution.cpp and just compare their results.

dendibakh avatar Aug 16 '21 17:08 dendibakh

cc @andrewevstyukhin

dendibakh avatar Aug 16 '21 17:08 dendibakh

Often in real cases not all code paths are covered. Any optimization can easily break something. It's a good honeypot in a too simple task to show reality. Anyway, commit 285f007e4f78bcc95bcdf97b25a7c631868ef66b prevents trival broken solution {}.

andrewevstyukhin avatar Aug 16 '21 17:08 andrewevstyukhin

Often in real cases not all code paths are covered. Any optimization can easily break something. It's a good honeypot in a too simple task to show reality. Anyway, commit 285f007 prevents trival broken solution {}.

So Andrey, do you suggest that we don't duplicate code from solution.cpp into validate.cpp ?

dendibakh avatar Aug 16 '21 18:08 dendibakh

I think we can use originalMultiply in validation.cpp to be more precise if current situation is so dramatic. Please try 285f007 first (just merge main into private/*)

andrewevstyukhin avatar Aug 16 '21 18:08 andrewevstyukhin

The problem is not just a validation, it's also about clear separation between client's and core code. Originally, I edited multiply() and identity() functions in my solution and was very surprised to find that they are used by someone else. I think that it should be clear what people can and cannot do. The simplest approach is to allow them to change only solution.h/cpp. For example existing solution.h:

// Assume this constant never changes
constexpr int N = 400;

// Square matrix 400 x 400
using Matrix = std::array<std::array<float, N>, N>;

void zero(Matrix &result);
void identity(Matrix &result);
void multiply(Matrix &result, const Matrix &a, const Matrix &b);
Matrix power(const Matrix &input, const uint32_t k);

void init(Matrix &matrix);

Should be just:

#include "problem.h" // Matrix and N are here now

Matrix power(const Matrix &input, const uint32_t k);

OleksandrKvl avatar Aug 16 '21 19:08 OleksandrKvl

Performance problems lay in solution.h and solution.cpp. Microoptimizations don't mean to use other algorithm or rewrite source code. Location of the performance issue is also unknown. Good job is to use information from the topic and use a profiler. All issues are topmost, not second order.

andrewevstyukhin avatar Aug 16 '21 19:08 andrewevstyukhin