[jvm-packages] Removing synchronized predict

[viswanathk]

predict method is now thread safe, so the predict method need not be synchronized. There's nothing else that maintains state in the Java world.

cc: @CodingCat

[CodingCat]

this LGTM

@trivialfis do we still have to be locked in c++ layer when doing prediction? and is the fix for the problem mentioned in #5272 also applying to the normal prediction method (or we have to use in_place_pred? )

[trivialfis]

@CodingCat

Inplace prediction is not used during training. For training it's always predicting on DMatrix.

do we still have to be locked in c++ layer when doing prediction

Yes. Only inplace prediction is lock free at the moment. Normal prediction can be made to lock free, but it might create some overhead during training as there's a work space for prediction (should be minor). From my perspective, for low latency/low memory usage prediction inplace predict is a better choice, so I didn't try to clear the lock for normal predict.

and is the fix for the problem mentioned in #5272 also applying to the normal prediction method

The fix was for normal prediction, as there's no cache for inplace predict right now. I restructured the cache implementation. So training from checkpoint should not have any issue.

[trivialfis]

Could you please rebase the branch to master?

[CodingCat]

@trivialfis

for xgb-spark, we are calling normal prediction, so it is eventually locked? not sure if removing synchronize can bring much improvements

I restructured the cache implementation. So training from checkpoint should not have any issue.

IIUC, so now the normal prediction will not go over all trees even without a prediction cache?

[trivialfis]

for xgb-spark, we are calling normal prediction, so it is eventually locked?

Yes.

not sure if removing synchronize can bring much improvements

Removing a lock in language binding is still an improvement as it's a duplicated lock.

IIUC, so now the normal prediction will not go over all trees even without a prediction cache?

It will rebuild the cache on first iteration, then works as usual.

[viswanathk]

for xgb-spark, we are calling normal prediction, so it is eventually locked? not sure if removing synchronize can bring much improvements

Yeah. I did not know there's a lock internally for predict, so this won't give as much performance boost. Removing it still makes sense though, in case there's a future expensive op before the lock, Java client will benefit from it.

@trivialfis Rebased.

[trivialfis]

Thanks, best scenario would be having inplace predict implemented for java. Just that I'm not familiar with the language's ecosystem so not sure what data to support.

[trivialfis]

Okay, the prediction contribution is not thread safe ...

[trivialfis]

I will make that function thread safe later.