expensive CHECK_GE

[okuvshynov]

Is this CHECK_GE useful for catching any potential bug?

xgboost/src/data/data.cc

Lines 653 to 658 in 91c6463

	if (!common::CheckNAN(element.value) && element.value != missing) {
	size_t key = element.row_idx - base_rowid;
	// Adapter row index is absolute, here we want it relative to
	// current page
	CHECK_GE(key, builder_base_row_offset);
	builder.AddBudget(key, tid);

It's fairly expensive for what it does; removing it reduces data loading time from ~9s to ~7s on Higgs dataset (with whole training time being ~1m on that machine, so, ~3% total.

call to LogCheck_GE is not inlined (gcc 7.4, x86):

#define CHECK_BINARY_OP(name, op, x, y)                               \
  if (dmlc::LogCheckError _check_err = dmlc::LogCheck##name(x, y))    \
    dmlc::LogMessageFatal(__FILE__, __LINE__).stream()                \
      << "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": "

disasm:

   cc7fa:       4d 2b 5a 10             sub    0x10(%r10),%r11
   cc7fe:       4c 89 5c 24 50          mov    %r11,0x50(%rsp)
   cc803:       e8 98 37 ff ff          callq  bffa0 <dmlc::LogCheckError dmlc::LogCheck_GE<unsigned long, unsigned long>(unsigned long const&, unsigned long const&)>
   cc808:       48 83 7c 24 58 00       cmpq   $0x0,0x58(%rsp)

and forcing inlining it will be likely not a good idea, as that function is pretty big.

If the check is needed, one way to improve would be to short-circuit the condition in the macro itself and avoid the call:

#define CHECK_BINARY_OP(name, op, x, y)                               \
+  if (!(x op y))    \
    if (dmlc::LogCheckError _check_err = dmlc::LogCheck##name(x, y))    \
    dmlc::LogMessageFatal(__FILE__, __LINE__).stream()                \
      << "Check failed: " << #x " " #op " " #y << *(_check_err.str) << ": "

That'd still be a 1-2 seconds win (on that, specific configuration). I can make a change, but if it's ok to just remove the check, that would be easier.

[trivialfis]

There was a patch in dmlc-core for reducing the overhead from checks. I believe it should help once we update dmlc-core. @hcho3 Correct me if I'm wrong.

[hcho3]

@trivialfis Yes, that's correct.

[okuvshynov]

dmlc/dmlc-core#613 ? I think it solves a little bit different issue; In any case, I've updated dmlc-core and ran test again with adding another if to avoid the call.

new dmlc	new dmlc + extra if
9.58641	7.81539
9.06665	7.60433
9.50086	7.36326
9.27835	7.70059
9.42832	6.92874
9.18233	8.43693
9.81083	7.23658
9.09448	7.29705
9.32441	7.67747
9.50768	7.29848

avg = 7.53s vs 9.37 (25% data loading time win).

I'll submit PR with that change to dmlc.